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
Allow option to be overwritten to allow subclassing #392
Conversation
@@ -37,7 +37,7 @@ def wrapped(plugin_class): | |||
# Already have some options set, e.g. because of subclassing | |||
# where both child and parent have a takes_config decorator | |||
for opt in result.values(): | |||
if opt.name in plugin_class.takes_config: | |||
if opt.name in plugin_class.takes_config and not opt.overwrite: | |||
raise RuntimeError( | |||
f"Attempt to specify option {opt.name} twice") | |||
plugin_class.takes_config = immutabledict({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hej thanks for the changes, one question though. I do not clearly see the overwrite part I have to admit. To me it looks like both options are now in plugin_class.takes_config
, but maybe I am wrong. Have you checked the lineage and that the option is propagated to the plugin correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @WenzDaniel , yes I checked, nevertheless not bad to just add a test: 5fc6f87
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow that was quick thanks
What is the problem / what does the code in this PR do
Not allowing options to be overwritten can lead to less clear code than the other way around. See: https://github.com/XENONnT/cutax/issues/3
Can you briefly describe how it works?
Allow an option to be overwritten with the expert flag
overwrite=True
.Can you give a minimal working example (or illustrate with a figure)?
Even better! I have a pull request:
https://github.com/XENONnT/cutax/pull/4