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

🐛 Ensure that the value of is_flag is passed correctly to Click #872

Closed
wants to merge 3 commits into from

Conversation

iurisilvio
Copy link

@iurisilvio iurisilvio commented Jun 27, 2024

Discussion in #873.

Click supports optional value.

To make it work, you have to set is_flag=False, flag_value="any value", then:

  • --foo return defined flag_value
  • --foo something return something

It was broken because is_flag was passed as None when parameter_info.is_flag was explicitly False.

@svlandeg svlandeg added bug Something isn't working p2 labels Jul 1, 2024
Copy link
Member

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this contribution! The discussion thread you opened in #873 clearly highlights the issue and the fact that the current Typer behaviour is inconsistent with how Click does it. Typer doesn't have any documentation around these specific parameters so it's fair to assume it would/should behave in the same way as Click does.

I could confirm the erratic behaviour and failing unit test on master, and with this PR the issues are resolved. The unit test perfectly captures the problems. The fix is minimal and accurate.

As such, I think this PR looks good to merge. Thanks again! 🙏

@svlandeg
Copy link
Member

Maybe one point to consider when Tiangolo reviews this, is the lack of Typer documentation for these settings. Perhaps we can add a separate page somewhere, explaining how these arguments work and relate to eachother. We could potentially do this in follow-up work though.

@svlandeg svlandeg changed the title Fix option optional value passing is_flag explicit False to click 🐛 Ensure that the value of is_flag is passed correctly to Click Jul 17, 2024
@tiangolo
Copy link
Member

tiangolo commented Nov 7, 2024

Thanks @iurisilvio!

I realized it didn't really make sense to me to have this option when it doesn't fit well with regular type annotations, I think it's better to have a simpler syntax for a boolean flag, and have it just be True or False. So chatting with @svlandeg, the conclusion was to drop this parameter completely to have one main way to achieve these things.

This was done in #987

Given that, I'll now close this one, but thanks for the effort! ☕

@tiangolo tiangolo closed this Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants