Skip to content

Cdat web plot subsetting#1951

Merged
doutriaux1 merged 16 commits intomasterfrom
cdat-web-plot-subsetting
Jun 22, 2016
Merged

Cdat web plot subsetting#1951
doutriaux1 merged 16 commits intomasterfrom
cdat-web-plot-subsetting

Conversation

@doutriaux1
Copy link
Copy Markdown
Contributor

@danlipsa this is the rest of your PR that you merged before I got a chance to push it. Please review.

Comment thread Packages/cdms2/Lib/avariable.py Outdated
elif key is Ellipsis:
raise CDMSError, "Misuse of ellipsis in specification."
elif type(key) is types.TupleType:
elif sinstance(key, tuple):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isinstance is misspeled here. Would flake8 catch this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep i need to run flake8 on this.

@danlipsa
Copy link
Copy Markdown
Contributor

danlipsa commented Apr 27, 2016

@doutriaux1 Nice cleanup! See my comments. @aashish24 Please review.

@danlipsa
Copy link
Copy Markdown
Contributor

@doutriaux1 What about the comments where I said it reverses the order. I don't think that happens actually, but rather it uses the default separator: ' ' rather than \n or ,

@doutriaux1
Copy link
Copy Markdown
Contributor Author

@danlipsa I commented on this, it doesn't reverse the order, look at my sample code in my comments.

In [1]: import string

In [2]: string.join(["A","B"],"-")
Out[2]: 'A-B'

In [3]: "-".join(["A","B"])
Out[3]: 'A-B'

@danlipsa
Copy link
Copy Markdown
Contributor

danlipsa commented Apr 28, 2016

@doutriaux1 I see. Confusing syntax for the join in statement 3! You would expect a member function of an object to be applied to the object, so the result would be - A B. Another possible interpretation is that we use "-" just to select between unicode and ascii so the result in that case is A B with the default separator. I like the syntax in 2 a lot better.

@danlipsa
Copy link
Copy Markdown
Contributor

danlipsa commented May 2, 2016

@doutriaux1 LGTM. 👍

@aashish24
Copy link
Copy Markdown
Contributor

@doutriaux1 looks like we need to update the branch to resolve the conflict.

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.

3 participants