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

Ensured Tree types do not create lowercase node on attribute access #2331

Merged
merged 3 commits into from Feb 26, 2018

Conversation

Projects
None yet
2 participants
@philippjfr
Copy link
Contributor

philippjfr commented Feb 10, 2018

As discussed in #1182, Tree based objects (e.g. Overlay and Layout) should not insert new lower-case nodes when the attribute is lower case.

@philippjfr philippjfr added the bug label Feb 10, 2018

@philippjfr philippjfr requested a review from jlstevens Feb 13, 2018

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Feb 13, 2018

I'm always nervous when I see changes to Tree behaviour. I agree with the changes as described but would you mind adding a few unit tests to check node insertion with lowercase names? There should already be tests inserting nodes starting with upper-case letters but if there aren't, a couple of those would be good to add as well.

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Feb 13, 2018

I agree with the changes as described but would you mind adding a few unit tests to check node insertion with lowercase names?

No sounds good, mostly wanted you to review first in case you had any immediate objections in mind.

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Feb 13, 2018

I'm wondering if a warning should be issued when this happens. Of course, lowercase method names on layouts/overlays must still work!

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Feb 13, 2018

I'm wondering if a warning should be issued when this happens.

It will trigger an AttributeError as it should.

Of course, lowercase method names on layouts/overlays must still work!

They do, they get handled first.

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Feb 13, 2018

Ok great. I'll merge once new unit tests are added and the tests are green.

@philippjfr philippjfr force-pushed the tree_attribute_lowercase branch from e2240ba to bc830e0 Feb 20, 2018

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Feb 20, 2018

Turns out that AttrTree didn't implement delitem even though some methods referenced it so I ended up implementing it. I've also added a number of tests for AttrTree.

@philippjfr philippjfr force-pushed the tree_attribute_lowercase branch 2 times, most recently from 55dbeb7 to a950853 Feb 20, 2018

Philipp Rudiger Philipp Rudiger

@philippjfr philippjfr force-pushed the tree_attribute_lowercase branch from a950853 to cba0c32 Feb 20, 2018

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Feb 20, 2018

Ready to merge.

@philippjfr philippjfr added this to the v1.10 milestone Feb 20, 2018

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

philippjfr commented Feb 26, 2018

Let's merge this now.

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Feb 26, 2018

Ok. Looks good and worth getting some real testing out of it.

@jlstevens jlstevens merged commit f3a5179 into master Feb 26, 2018

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 82.099%
Details
s3-reference-data-cache Tests passing no test data changes required.
Details

@philippjfr philippjfr deleted the tree_attribute_lowercase branch Mar 9, 2018

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.