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

Added recursive update and copy methods #44

Closed
wants to merge 4 commits into from

Conversation

sbillaudelle
Copy link

Added support for recursively merging dict-like stuff into a Dict instance.

old = Dict()
old.foo.a = 1
old.foo.b = 2
old.bar = 42

new = Dict()
new.foo.b = 3
new.foo.c = 'new kid on the block'
new.bar.asd = 42

old.update(new)

print old

… gives:

{'foo': {'a': 1, 'c': 'new kid on the block', 'b': 3}, 'bar': {'asd': 42}}

@coveralls
Copy link

Coverage Status

Coverage decreased (-5.21%) to 94.79% when pulling 4ced423 on sbillaudelle:master into 055d7cd on mewwts:master.

@mewwts
Copy link
Owner

mewwts commented Feb 6, 2015

Hi @sbillaudelle, thanks for the pull request. 👍
Would you mind writing out some tests for this?
Also, what happens when you have dictionaries inside, say, a list? Other methods recursively turns those into addicts as well. 🎱

@sabhiram
Copy link
Contributor

sabhiram commented Feb 6, 2015

Please make sure the coveralls coverage is back to 100% (should tell you what tests you should be writing 👍)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 637a6bd on sbillaudelle:master into 055d7cd on mewwts:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4e001e2 on sbillaudelle:master into 055d7cd on mewwts:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4e001e2 on sbillaudelle:master into 055d7cd on mewwts:master.

@sbillaudelle
Copy link
Author

I implemented a simple and far from complete test case. Just checks the basic functionality.

@mewwts, what do you mean by dictionaries inside a list?

@sbillaudelle
Copy link
Author

I'm not quite sure why the Travis CI build failed… Does not seem to be an issue with my commits, though.

@mewwts
Copy link
Owner

mewwts commented Feb 6, 2015

Thanks @sbillaudelle, it seems like pip install coveralls failed on 3.4.2. I'll have a look over the weekend.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 0a327e0 on sbillaudelle:master into 055d7cd on mewwts:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 0a327e0 on sbillaudelle:master into 055d7cd on mewwts:master.

@sbillaudelle sbillaudelle changed the title Added recursive update method Added recursive update and copy methods Feb 10, 2015
@mewwts
Copy link
Owner

mewwts commented Feb 10, 2015

Hi @sbillaudelle! The copy function was neet 👍
Is this PR behaviour consistent with #25 or do they differ?

@mewwts
Copy link
Owner

mewwts commented Feb 10, 2015

Also, if it is consistent with #25, should it be named extend instead?
Also to answer a previous question.
What happens in this case:

>>> old = Dict()
>>> old.a = 2

>>> new = Dict()
>>> new.b = {'a': 2} # regular dict

>>> old.update(new)
>>> old
{'a': 2, b: {'a': 2}}

At this point I think doing old.b.a will result in a AttributeError. I think today I'd expect the behaviour to be turning dicts into addicts, but that might change soon as I am thinking heavily about it, see #29. Feel free to post an comment on that issue. I'd love to hear your thoughts 👍

@sbillaudelle
Copy link
Author

@mewwts, currently old.b.a will correctly return 2.

@mewwts
Copy link
Owner

mewwts commented Feb 10, 2015

Oh, woops! Was a bit fast in constructing that example. I meant

>>> old = Dict()
>>> old.a = 2

>>> new = Dict()
>>> new.b = [{'a': 2}] # regular dict

>>> old.update(new)
>>> old
{'a': 2, b: [{'a': 2}]}

Then a.b[0].a will raise AttributeError. However I saw your comment to #29 and I realize that we can solve this issue.

@sbillaudelle
Copy link
Author

Yes, you are right about that one, @mewwts! In the end, this will be a design decision.

@mewwts
Copy link
Owner

mewwts commented Feb 10, 2015

I agree @sbillaudelle. Just want to make sound and consistent decisions at this point. Please excuse me for needing some time with this PR. This is merely a time where addict might undergo big changes. Appreciate your thought-out comment over at #29.

@mewwts
Copy link
Owner

mewwts commented Mar 11, 2015

This is rebased and merged, don't know why it's not saying so. Thanks for helping out @sbillaudelle, I appreciate it and hope you continue to do so 👍

Also, I was wrong about my example. Regular dictionaries are turned into Dicts because of the setitem syntax :-).

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.

None yet

4 participants