Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(#2318): install CLI by default #2346

Merged
merged 25 commits into from
Sep 27, 2023
Merged

feat(#2318): install CLI by default #2346

merged 25 commits into from
Sep 27, 2023

Conversation

lsanpablo
Copy link
Contributor

@lsanpablo lsanpablo commented Sep 23, 2023 •
edited
Loading

Pull Request Checklist

  • New code has 100% test coverage
  • (If applicable) The prose documentation has been updated to reflect the changes introduced by this PR
  • (If applicable) The reference documentation has been updated to reflect the changes introduced by this PR

Description

  • Install required CLI dependencies and CLI script as part of the base package
  • Update docs to reflect above
  • Add jsbeautifier group
  • Handle missing optional dependencies gracefully in the litestar cli
  • Add deprecation message to the CLI group in the docs

Close Issue(s)

@lsanpablo lsanpablo changed the title Install CLI by default ci: install CLI by default Sep 23, 2023
@JacobCoffee JacobCoffee changed the title ci: install CLI by default feat(#2318): install CLI by default Sep 24, 2023
@provinzkraut
Copy link
Member

There are a few things:

  1. Do we want to include uvicorn by default? I think we should not, and instead revisit support for different servers in the run command
  2. Should jsbeautifier be included by default? IMO we can have an extra for this, and if it's not present, just don't use it. It can go in the standard extra.
  3. The cli group should remain for now, to not break things. Not sure if poetry allows empty groups though

@litestar-org/maintainers @litestar-org/members thoughts?

@peterschutt
Copy link
Contributor

Should jsbeautifier be included by default? IMO we can have an extra for this, and if it's not present, just don't use it. It can go in the standard extra.

+1 for not default

@JacobCoffee
Copy link
Member

JacobCoffee commented Sep 24, 2023 •
edited
Loading 

1. Do we want to include `uvicorn` by default?

No

2. Should `jsbeautifier` be included by default?

No

3. The `cli` group should remain for now, to not break things. Not sure if poetry allows empty groups though

Agreed until V3

JacobCoffee and others added 2 commits September 24, 2023 14:27
@JacobCoffee
Merge branch 'main' into install_cli_by_default
0ced34e
@lsanpablo
feat: make uvicorn and jsbeautifier optional again
aaff5d7 
Addressed feedback to the parent PR

feat: add `jsbeautifier` group

docs: add section about the `standard` extra

docs: add deprecation message to CLI group

feat: gracefully handle optional dependencies from standard group in `litestar`
@sourcery-ai sourcery-ai bot mentioned this pull request Sep 25, 2023
Closed
@JacobCoffee
Copy link
Member

@lsanpablo Please see the Sourcery suggested refactorings:

cdff023

@lsanpablo
Copy link
Contributor Author

lsanpablo commented Sep 25, 2023 •
edited
Loading

Thanks for the feedback. I made the following changes:

  • re-added the cli group which includes jsbeautifier and uvicorn to offer equivalent functionality to what it was before
  • added the jsbeautifier group
  • refactored the litestar run and litestar schema typescript commands to gracefully handle the fact that the script may be installed but uvicorn and jsbeautifier might not
  • updated the documentation accordingly

@lsanpablo lsanpablo marked this pull request as ready for review September 25, 2023 04:53
JacobCoffee
JacobCoffee requested changes Sep 26, 2023
View reviewed changes
docs/index.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
litestar/cli/commands/core.py Outdated Show resolved Hide resolved
@JacobCoffee
Copy link
Member

Looks good except for a few comments

JacobCoffee and others added 7 commits September 25, 2023 22:21
@JacobCoffee
Update litestar/cli/commands/core.py
e15a150
@lsanpablo @JacobCoffee
Update docs/index.rst
e90cefd 
Co-authored-by: Jacob Coffee <[email protected]>
@lsanpablo @JacobCoffee
Update docs/index.rst
047c327 
Co-authored-by: Jacob Coffee <[email protected]>
@lsanpablo
docs: remove extra whitespace
26d6589
@lsanpablo
docs: clarify deprecation message
bd1d5dd 
Clarify deprecation message to imply that's it's deprecated now, and will likely be removed in v3

docs: add code formatting to `litestar`
@lsanpablo
docs: add missing space
a60ef5e
@lsanpablo
Merge branch 'main' into install_cli_by_default
e463f65
@lsanpablo
Copy link
Contributor Author

Setting PR to draft while I resolve the poetry.lock merge conflict.

@lsanpablo lsanpablo marked this pull request as draft September 27, 2023 22:38
@github-actions
Copy link

Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/2346

@lsanpablo lsanpablo marked this pull request as ready for review September 27, 2023 23:04
@JacobCoffee
Copy link
Member

lgtm. sorry for all the nits 😛

@JacobCoffee JacobCoffee merged commit ab84566 into litestar-org:main Sep 27, 2023
15 checks passed
@JacobCoffee
Copy link
Member

@all-contributors add @lsanpablo code tests docs

@allcontributors allcontributors bot mentioned this pull request Sep 27, 2023
Merged
@allcontributors
Copy link
Contributor

@JacobCoffee

I've put up a pull request to add @lsanpablo! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JacobCoffee JacobCoffee JacobCoffee approved these changes

@provinzkraut provinzkraut Awaiting requested review from provinzkraut provinzkraut is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enhancement: Make litestar CLI part of the base package
4 participants
@lsanpablo @provinzkraut @peterschutt @JacobCoffee