-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Added exception to avoid option without value in default_options #3245
Conversation
For me, it was a bug, but we are going to break recipes. Should we consider to adjust "" to "None"? Makes sense? |
In that case the adjustment should only be for "ANY" but I'd say it would be an ugly patch for a non-desired usage that we dont want to promote. There should not be many people using "ANY" for options and in that case defining "config=" in |
def configure(self): | ||
if self.options.config: | ||
self.output.info("Boolean evaluation") | ||
if self.options.config is None: |
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.
if self.options.config == None
should work, please test it.
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.
This is not working because of this line here:
Line 327 in 5aa4cfd
if other is None: |
However behavior could be changed as I think this is not documented.
conans/model/options.py
Outdated
@@ -270,6 +273,9 @@ def loads(text): | |||
if not line: | |||
continue | |||
name, value = line.split("=", 1) | |||
if not value.strip(): |
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.
Please do one single strip()
and reuse it.
Well, me and my coworkers were one of the "not many people" who used both "ANY" and default_option with nothing after the "=" 😅. Personally I didn't even thought about the logic of this, but somehow it seemed ok, since I wanted that option by default to be an empty string. So I saw this as a feature.
options = {
"option_ANY": "ANY",
"option_VALUES": ['SomeValue', 'SomeOtherValue', 'None']
}
default_options = (
"option_ANY=None",
"option_VALUES=None"
)
if self.options.option_ANY:
#This will be executed
if self.options.option_VALUES:
#This won't be executed ? I think this can be kinda confusing. |
Yes, sorry for that. When it was stated that
Actually some research was done, and we couldn't find any relevant usage, so it seemed more a bug than a feature. Thanks for your feedback, we will re-consider it and discuss as high priority, and report back asap. Cheers! |
Revert "Merge pull request #3245 from danimtb/feature/3232"
Ok. Thank you for your promptness, help and submission to the stability commitment 😌 . |
Hi @Sebiee |
Updated and tried. Everything works ok now. Thank you! |
Added exception to avoid option without value in default_options
Revert "Merge pull request conan-io#3245 from danimtb/feature/3232"
develop
branch, documenting this one. Also adding a description of the changes in thechangelog.rst
file. https://github.com/conan-io/docsFinally this PR will not make the value type as NoneType when using "config=None", just raise a exception when "config="