Skip to content

Fix default value for flags with multiple=True#2247

Closed
amyreese wants to merge 1 commit intopallets:8.1.xfrom
amyreese:flag-eager-multiple
Closed

Fix default value for flags with multiple=True#2247
amyreese wants to merge 1 commit intopallets:8.1.xfrom
amyreese:flag-eager-multiple

Conversation

@amyreese
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

davidism commented Apr 5, 2022

Yes, that seems better.

@amyreese amyreese closed this Apr 5, 2022
@amyreese
Copy link
Copy Markdown
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
Copy Markdown
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.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@amyreese @davidism