-
Notifications
You must be signed in to change notification settings - Fork 68
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
Cdat web plot subsetting #1951
Cdat web plot subsetting #1951
Conversation
slicelist.append(key) | ||
elif key is unspecified or key is None or key == ':': | ||
slicelist.append (slice(0, len(self.getAxis(i)))) | ||
elif key is Ellipsis: | ||
raise CDMSError, "Misuse of ellipsis in specification." | ||
elif type(key) is types.TupleType: | ||
elif sinstance(key, tuple): |
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.
isinstance is misspeled here. Would flake8 catch this?
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.
yep i need to run flake8 on this.
@doutriaux1 Nice cleanup! See my comments. @aashish24 Please review. |
@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 , |
@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' |
@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. |
@doutriaux1 LGTM. 👍 |
@doutriaux1 looks like we need to update the branch to resolve the conflict. |
@doutriaux1 @danlipsa should we update this branch and merge it? |
@doutriaux1 @danlipsa same here. I believe we all did LGTM. Any hold on this branch? |
@danlipsa this is the rest of your PR that you merged before I got a chance to push it. Please review.