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

Added exception to avoid option without value in default_options #3245

Merged
merged 4 commits into from
Aug 27, 2018

Conversation

danimtb
Copy link
Member

@danimtb danimtb commented Jul 20, 2018

  • Refer to the issue that supports this Pull Request: closes Default value for options with ANY #3232
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one. Also adding a description of the changes in the changelog.rst file. https://github.com/conan-io/docs

Finally this PR will not make the value type as NoneType when using "config=None", just raise a exception when "config="

Sorry, something went wrong.

@danimtb danimtb added this to the 1.7 milestone Jul 20, 2018
@ghost ghost assigned danimtb Jul 20, 2018
@ghost ghost added the stage: review label Jul 20, 2018
@danimtb danimtb removed their assignment Jul 20, 2018
@lasote
Copy link
Contributor

lasote commented Jul 23, 2018

For me, it was a bug, but we are going to break recipes. Should we consider to adjust "" to "None"? Makes sense?

@danimtb
Copy link
Member Author

danimtb commented Jul 23, 2018

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 default_options seems a no-sense to me. I'd rather break to state that his is a not allowed usage.

@lasote lasote requested a review from memsharded July 24, 2018 06:47
def configure(self):
if self.options.config:
self.output.info("Boolean evaluation")
if self.options.config is None:
Copy link
Member

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.

Copy link
Member Author

@danimtb danimtb Aug 23, 2018

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:

if other is None:

However behavior could be changed as I think this is not documented.

@@ -270,6 +273,9 @@ def loads(text):
if not line:
continue
name, value = line.split("=", 1)
if not value.strip():
Copy link
Member

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.

@ghost ghost assigned danimtb Aug 23, 2018
@memsharded memsharded merged commit 1f740fa into conan-io:develop Aug 27, 2018
@ghost ghost removed the stage: review label Aug 27, 2018
@Sebiee
Copy link

Sebiee commented Sep 3, 2018

There should not be many people using "ANY" for options and in that case defining "config=" in default_options seems a no-sense to me.

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.
3 things i want to mention:

  1. In the company I work for, we integrated conan for a lot of our projects. We really like this package manager and appreciate the work you are doing. 👍
    However, introducing a change that would break recipes that would work with a version earlier seems kinda rough. In future we would highly appreciate some warnings like "deprecated, won't work in future versions" or something. It would help us organize our time in resolving things 😌 .

  2. With the current implementation how can I default an "ANY" option to be empty string by default? Is this not allowed?

  3. Why does those 2 approaches produce different results:

 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.

@memsharded
Copy link
Member

However, introducing a change that would break recipes that would work with a version earlier seems kinda rough. In future we would highly appreciate some warnings like "deprecated, won't work in future versions" or something. It would help us organize our time in resolving things

Yes, sorry for that. When it was stated that

There should not be many people using "ANY" for options and in that case defining "config="

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!

jgsogo added a commit to jgsogo/conan that referenced this pull request Sep 4, 2018
This reverts commit 1f740fa, reversing
changes made to 32c280c.
memsharded added a commit that referenced this pull request Sep 4, 2018
Revert "Merge pull request #3245 from danimtb/feature/3232"
@memsharded
Copy link
Member

Hi @Sebiee

We have reverted this change in #3472, and will be releasing the fix asap in conan 1.7.2. Sorry for the inconvenience!

@Sebiee
Copy link

Sebiee commented Sep 4, 2018

Ok. Thank you for your promptness, help and submission to the stability commitment 😌 .

@memsharded
Copy link
Member

Hi @Sebiee
1.7.2 was released yesterday, please update and try. Thanks!

@Sebiee
Copy link

Sebiee commented Sep 5, 2018

Updated and tried. Everything works ok now. Thank you!

grisumbras pushed a commit to grisumbras/conan that referenced this pull request Dec 27, 2018
 Added exception to avoid option without value in default_options
grisumbras pushed a commit to grisumbras/conan that referenced this pull request Dec 27, 2018
This reverts commit 1f740fa, reversing
changes made to 32c280c.
grisumbras pushed a commit to grisumbras/conan that referenced this pull request Dec 27, 2018
Revert "Merge pull request conan-io#3245 from danimtb/feature/3232"
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.

Default value for options with ANY
4 participants