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

Fix default value for flags with multiple=True #2247

Closed
wants to merge 1 commit into from

Conversation

amyreese
Copy link
Contributor

@amyreese amyreese commented Apr 5, 2022

Sets a default value (empty tuple) for flags with multiple=True. Adds a test case (that fails on 8.1.2) to help prevent future regression.

Fixes #2246

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

@amyreese
Copy link
Contributor Author

amyreese commented Apr 5, 2022

I'm happy to add the changes documentation, just wasn't sure what wording would be preferred for that.

@davidism
Copy link
Member

davidism commented Apr 5, 2022

Looking at this example, I'm not sure this was ever intended? You should use count=True if you want a flag without value that can be specified multiple times.

@amyreese
Copy link
Contributor Author

amyreese commented Apr 5, 2022

Looking at this example, I'm not sure this was ever intended? You should use count=True if you want a flag without value that can be specified multiple times.

Hmm, I was just trying to test count, and got this error message:

TypeError: 'count' is not valid with 'is_flag'.

Perhaps it would be better to explicitly disallow is_flag=True and multiple=True in the same manner, rather than waiting for a vague error deep in the parsing code path.

@davidism
Copy link
Member

davidism commented Apr 5, 2022

Yes, that seems better.

@amyreese amyreese closed this Apr 5, 2022
@amyreese
Copy link
Contributor Author

amyreese commented Apr 5, 2022

Opened #2248 with alternative change.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2022
@davidism
Copy link
Member

davidism commented Jul 3, 2023

Well, in the end I decided to go with this solution instead, see #2550 😅

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants