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

Give a UserWarning when Parameter is overriden by name #2397

Merged
merged 10 commits into from
Nov 3, 2024

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
39 tasks
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue UserWarning when overriding Parameter name
4 participants