Skip to content

Conversation

edrogers
Copy link
Contributor

@edrogers edrogers commented Nov 7, 2022

This adds a UserWarning when two parameters on one command have a name conflict. Very interested in thoughts and feedback.

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.

@emlowe
Copy link

emlowe commented Dec 15, 2022

This issue recently caused Chia a little grief when a short name collision was merged in - so I'm in support of this concept.

@davidism davidism added this to the 8.2.0 milestone Jul 4, 2023
@AndreasBackx AndreasBackx mentioned this pull request Oct 20, 2024
34 tasks
@AndreasBackx AndreasBackx force-pushed the check-for-option-override branch from 8778b31 to 92be37c Compare November 3, 2024 13:38
@AndreasBackx AndreasBackx force-pushed the check-for-option-override branch from cca0c46 to b9f0ae3 Compare November 3, 2024 13:39
@AndreasBackx
Copy link
Collaborator

AndreasBackx commented Nov 3, 2024

The implementation proposed here checks every single time an option is used, whether its name has been used before. I moved this check to Command.get_params so it only does this check once on command execution. The downside is that this will only be shown when the command is run, though this seems like a valid trade-off.

Additionally, the error message does not tell you which parameters. That could technically be done, but I'm not sure it's worth the extra time spent as it should be relatively straightforward to figure out.

@AndreasBackx AndreasBackx merged commit 3800083 into pallets:main Nov 3, 2024
12 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 20, 2024
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.

Issue UserWarning when overriding Parameter name
4 participants