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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from Feb 26, 2018
Merged

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

merged 3 commits into from Feb 26, 2018

Conversation

philippjfr
Copy link
Member

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 type: bug Something isn't correct or isn't working label Feb 10, 2018
@jlstevens
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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

@philippjfr
Copy link
Member Author

philippjfr commented Feb 13, 2018 •
edited

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
Copy link
Contributor

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

@philippjfr
Copy link
Member Author

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 Compare February 20, 2018 02:10
@philippjfr
Copy link
Member Author

Ready to merge.

@philippjfr philippjfr added this to the v1.10 milestone Feb 20, 2018
@philippjfr
Copy link
Member Author

Let's merge this now.

@jlstevens
Copy link
Contributor

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

@jlstevens jlstevens merged commit f3a5179 into master Feb 26, 2018
@philippjfr philippjfr mentioned this pull request Feb 27, 2018
Merged
@philippjfr philippjfr deleted the tree_attribute_lowercase branch March 9, 2018 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlstevens jlstevens Awaiting requested review from jlstevens

Assignees
No one assigned
Labels
type: bug Something isn't correct or isn't working
Projects
None yet
Milestone
v1.10
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@philippjfr @jlstevens