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

Nested dicts create nested attributes on Dict(), but not on update() #105

Open
gnudiff opened this issue Dec 19, 2018 · 8 comments
Open

Comments

@gnudiff
Copy link

gnudiff commented Dec 19, 2018

pip installed addict 2.2.0 on Python 2.7.12/Ubuntu 16.04.5 LTS

If you create Dict() with initial data, all the nested dicts are converted to attributes.
If you update an existing Dict(), the nested dicts remain dicts and nested attributes are not created.

[This, by the way, is contrary to what happens with the two analogues AttrDict and EasyDict. Those two, on the other hand, do not support creation of nested attributes on the fly at assignment, like d=Dict() d.a.b.c=1 ]

>>> from addict import Dict
>>> d={'A': 1, 'B': { 'C':2, 'D':3 }}
>>> x=Dict(d)
>>> x.B
{'C': 2, 'D': 3}
>>> x.B.C
2
>>> y=Dict()
>>> y.update(d)
>>> y.B # appears similar
{'C': 2, 'D': 3}
>>> y.B.C
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'dict' object has no attribute 'C'

@mewwts
Copy link
Owner

mewwts commented Dec 20, 2018

Hey @gnudiff - thanks for reaching out.

In previous versions of addict we would convert "everything" to an instance of a Dict. This led to some surprising results where the items you put into a Dict was in fact not referenced by the Dict (since the Dict held a copy). This is why the constructor is the only part of addict which will recursively transform a dict to a Dict.

This is the intended, although I guess sometimes, surprising, functionality! In order to get the dict nested, try doing y.update(Dict(d)).

Hope this helps!

@mewwts
Copy link
Owner

mewwts commented Dec 20, 2018

Also see #95

@mewwts mewwts closed this as completed Dec 20, 2018
@mewwts mewwts reopened this Dec 20, 2018
@gnudiff
Copy link
Author

gnudiff commented Dec 20, 2018

In that case, I would recommend putting this info in either project description or update method docstring, as it could be considered unexpected behaviour, and appears confusing (and as I mentioned, it is not the case with eg. AttrDict alternative, which does create recursive AttrDict objects on update).

In case you could be curious, my personal case was that I wanted to use addict for dotted access to JSON-stored configuration files, with possibility of parsing config from several files sequentially. For that, I tried just to extend Dict with load() and save() methods, of which load() would call Dict.update()

@mewwts
Copy link
Owner

mewwts commented Dec 20, 2018

Awesome use case - have done that myself.

As for your recommendations - they are noted. It is mentioned, but perhaps not explicitly enough.

Thanks for using addict!

@doconix
Copy link

doconix commented Jan 29, 2019

+1

Just another vote for this. Overall, I really like your implementation, but the constructor and update having different behavior was a gotcha for me. It seems like they should be consistent with one another.

Perhaps a kwarg on update that allows the user to select whether Dict is deep or not?

@mewwts
Copy link
Owner

mewwts commented Jan 31, 2019

Thanks @doconix.

I understand that this creates confusion. I'd be open to looking into changing it in a future major release, but I'm torn on exactly the best API is.

There's four(?) ways to set attributes and items on a addict today

a.b = 2
a['b'] = 2
a = Dict(somedict)
a.update(otherdict)

and I'd love for them to be consistent, while at the same time supporting both mutating and not mutating references to the objects being set.

The kwarg suggestion is a good one - thanks. If you'd like, I'd happy to consider any pull requests for change of functionality, better documentation or anything else you see missing. However, I can not promise a merge of backwards-breaking functionality at this point.

Thanks again!

@mewwts
Copy link
Owner

mewwts commented Sep 12, 2020

@gnudiff @doconix any interest in attempting to find a consisten API across the four methods I outlined above? As mentioned happy to receive a PR.

@sumukhbarve
Copy link
Contributor

sumukhbarve commented Nov 26, 2020

Hi @mewwts,

Continuing our discussion in #129, after #130, this #105 seems to be a good candidate to work on next. What do you think?

As @doconix mentions, the fact that .__init__() and .update() behave differently is a bit weird. And while documentation adds clarity, it doesn't really make such behavior fully non-weird.

Just for a quick comparison, in Dotsi and EasyDict, .update(), .__init__(), .__setattr__() and .__setitem__() all work the same way. And with regard to #95, Dotsi retains references too.

In Addict, couldn't .__init__() and .update() be implemented as aliases? Or, could one call the other? I realize that we'll need to be careful about backward compatibility, but could you please indicate the specific compatibility issues we'll need to be vigilant of? Clarity regarding which aspects should be preserved would be helpful.

Regards,
S

Edit: Also, could you please clarify if the following .update() behavior is desired:

>>> x = dict({"foo": {"bar": "baz"}})
>>> x.update({"foo": {"hello": "world"}})
>>> x
{'foo': {'hello': 'world'}}
>>> 
>>> from addict import Dict
>>> y = Dict({"foo": {"bar": "baz"}})
>>> y.update({"foo": {"hello": "world"}})
>>> y
{'foo': {'bar': 'baz', 'hello': 'world'}}
>>> 

Thanks!

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

No branches or pull requests

4 participants