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

Allow specifying default_options as a dictionary (close #2713) #3477

Merged
merged 22 commits into from Oct 8, 2018

Conversation

Projects
None yet
3 participants
@jgsogo
Copy link
Member

commented Sep 4, 2018

  • Issue related: close #2713
  • 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

This PR requires #3424 to be merged first

Changelog: Feature: The default_options in a conanfile.py can be specified now as a dictionary.

@ghost ghost assigned jgsogo Sep 4, 2018

@ghost ghost added the stage: review label Sep 4, 2018

values = [(k, v) for k, v in values.items()]

# Check that we have a list of tuples
offenders = list(filter(lambda u: not isinstance(u, (tuple, list)) or len(u) <= 1, values))

This comment has been minimized.

Copy link
@memsharded

memsharded Sep 4, 2018

Contributor

overly protective/defensive, not needed. I'd try to apply the "python zen" whenever possible. Also, do not check types and do defensive programming unless there is strong evidence from users/usage that it is needed because of frequent error.

In python, a = {1: 2, 1: 2} is valid, won't raise an error.

This helps to keep the code as simple as possible, future maintenance is very important.

This comment has been minimized.

Copy link
@jgsogo

jgsogo Sep 5, 2018

Author Member

Here (L179) I implement in just a single point that every option has at least a value and build a meaningful message to the user. I don't see where is the over protection. But we should better talk about it here (#3424 (review)).

About the dict you propose (with non-string keys) it will raise a weird error in line L192, but I'm not protecting against it because I think that an option name will always be a string.

This comment has been minimized.

Copy link
@memsharded

memsharded Oct 1, 2018

Contributor

I still don't see it. If necessary, convert dict entries to strings on input, instead of checking them and raising, it would be more consistent to the current approach.

This comment has been minimized.

Copy link
@jgsogo

jgsogo Oct 1, 2018

Author Member

These lines of code come from the PR #3424 which has to be merged first. And the lines you don't like are already removed. I will merge that branch now to update it here.

@@ -160,21 +163,38 @@ def __init__(self, values=None):
if not values:
return

assert not isinstance(values, six.string_types), "String type not expected in OptionValues " \

This comment has been minimized.

Copy link
@memsharded

memsharded Sep 4, 2018

Contributor

Please use a if - elif structure to process all posible types.
I would allow string type, and would handle as single element tuple. It is reasonable that someone will write:

default_options = "shared=True"

This comment has been minimized.

Copy link
@jgsogo

jgsogo Sep 5, 2018

Author Member

default_options are parsed using this function where the value "shared=True" will enter through OptionsValues.loads(...) so in OptionsValues.__init__ we are never expecting a single string. We can refactor all this code, in fact I agree that it is more complex than it should be.

@ghost ghost assigned jgsogo Oct 1, 2018


# handle list of tuples (name, value)
for (k, v) in values:
k = k.strip()
v = v.strip() if isinstance(v, six.string_types) else v

This comment has been minimized.

Copy link
@memsharded

memsharded Oct 8, 2018

Contributor

I still think that this line is dangerous. It will slip non-string values into the options model, which so far, was restricted. I think that it should be enforced above (in the dict input) that the values are already strings.

This comment has been minimized.

Copy link
@jgsogo

jgsogo Oct 8, 2018

Author Member

That's another issue, previously non-string values could enter through the tuple interface... if we want all to be strings then something else could change (see #3620 and the None)

@memsharded memsharded merged commit 94a4677 into conan-io:develop Oct 8, 2018

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the stage: review label Oct 8, 2018

@jgsogo jgsogo deleted the jgsogo:issue/2713 branch Oct 8, 2018

grisumbras pushed a commit to grisumbras/conan that referenced this pull request Dec 27, 2018

Merge pull request conan-io#3477 from jgsogo/issue/2713
Allow specifying default_options as a dictionary (close conan-io#2713)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.