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.

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

Co vs CO #43

Merged
merged 7 commits into from
Apr 25, 2023
Merged

Co vs CO #43

merged 7 commits into from
Apr 25, 2023

Conversation

avaldebe
Copy link
Collaborator

@avaldebe avaldebe commented Mar 20, 2023

closes #42

Not sure how to address this issue.
My first try was to exclude from the CLI thge pollutants that would confuse typer.
Currently, I use click to construct the sub commands.

@avaldebe avaldebe added the bug Something isn't working label Mar 20, 2023
@avaldebe avaldebe requested a review from JohnPaton March 20, 2023 13:25
@avaldebe avaldebe self-assigned this Mar 20, 2023
@avaldebe avaldebe marked this pull request as ready for review March 20, 2023 13:45
@avaldebe
Copy link
Collaborator Author

please note that the first commit on this PR is the same as #44

@avaldebe avaldebe force-pushed the CO-vs-Co branch 2 times, most recently from 975044a to 9e87071 Compare March 20, 2023 15:53
@JohnPaton
Copy link
Owner

Does it even make sense to keep typer as a dependency with this change? Seems like we're entirely working around it and essentially just using click (which is also totally fine, I like click too)

@avaldebe
Copy link
Collaborator Author

Does it even make sense to keep typer as a dependency with this change? Seems like we're entirely working around it and essentially just using click (which is also totally fine, I like click too)

Writing a CLI with typer is much easier than with click, but I see your point.
It really depends if typer fixes the problem, see fastapi/typer/discussions/570.
The fix was relatively simple so I submitted a PR fastapi/typer#571.

We can wait for the typer PR to be merged or move to click now, please let me know what you prefer and I'll deal with it.

@avaldebe avaldebe changed the title Co vs co Co vs CO Apr 13, 2023
@avaldebe
Copy link
Collaborator Author

We can wait for the typer PR to be merged or move to click now, please let me know what you prefer and I'll deal with it.

Now that I think more about it, I'll implement the move to pure click while I wait for your answer. I do not mind to revert to typer after they fix the problem, we can keep using click if you are happy with the pure click implementation.

@JohnPaton
Copy link
Owner

I +1'd your PR, hopefully it makes it in!

@JohnPaton
Copy link
Owner

The click changes look good, I'm actually surprised how similar they are (though agreed typer is easier when it works)

@avaldebe avaldebe merged commit 530dfd9 into JohnPaton:master Apr 25, 2023
@avaldebe
Copy link
Collaborator Author

Thanks for approving the PR. I will prepare a new tag/release...

@JohnPaton
Copy link
Owner

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cli request confuses "CO" with "Co"
2 participants