Catch dict-like vals in __setitem__, apply from_dict()

[mpacer]

Directly addresses feature in #102.

only difference from the suggested implementation by @minrk is that I added this to __setitem__ instead of __setattr__. I did this because __setattr__ was calling __setitem__ anyway and this also covers the case where assignment is accomplished standard the standard object["key"] syntax. This is already supported by NotebookNode (it inherits from ipython_genutils.ipstruct.Struct which is is a dictlike & has a __setitem__ implementation).

My one concern is that tab completion (in the notebook) is not catching attribute values set in this way (even if they are set using the attribute syntax, which is odd).

[mpacer]

Not sure where/if to test this explicitly as there are no extant tests that specifically work to check for NotebookNode functionality.

[mpacer]

One thing that might be nice would also be to change it so that when NotebookNode().update is called it similarly applies the same recursive logic. Hrmm…

[minrk]

Instead of re-implementing this check, we can use super:

if isinstance(value, dict):
    value = from_dict(value)
return super(NotebookNode, self).__setitem__(key, value)

[mpacer]

Changed the check for dict to a check for Mapping since dict is (somehow?) a subclass of collections.Mapping (see this SO post for more updates on the how/why).

[minrk]

One thing that might be nice would also be to change it so that when NotebookNode().update

👍

def update(self, other):
    for k, v in other.items():
        self[k] = v

should trigger this setitem logic.

[mpacer]

@minrk Ok, if we're ok going with that I'll implement it. I was having difficulty figuring out what the actual signature for update should be…

The supposed signature (in CPython) was

D.update([E, ]**F) -> None.  Update D from dict/iterable E and F.
If E is present and has a .keys() method, then does:  for k in E: D[k] = E[k]
If E is present and lacks a .keys() method, then does:  for k, v in E: D[k] = v
In either case, this is followed by: for k in F:  D[k] = F[k]

But that seems bizarrely complicated and not at all like the documented signature

update([other])
Update the dictionary with the key/value pairs from other, overwriting existing keys. Return None.

update() accepts either another dictionary object or an iterable of key/value pairs (as tuples or other iterables of length two). If keyword arguments are specified, the dictionary is then updated with those key/value pairs: d.update(red=1, blue=2).

and with no implementation in python (since it's part of builtins) I spent a while looking for the dict implementation in pypy which has been surprisingly difficult to figure trace down. I tried looking also for an implementation in any of the IPython codebase… but that only had update for displays (which is not relevant for this problem).

So, I don't think that this will deal with the multiple keys issue, but I'll implement it anyway. If people run into an issue with how they expect to use it, we can fix it then.

[mpacer]

Aha, found this implementation in the _collections_abc module:

    def update(*args, **kwds):
        ''' D.update([E, ]**F) -> None.  Update D from mapping/iterable E and F.
            If E present and has a .keys() method, does:     for k in E: D[k] = E[k]
            If E present and lacks .keys() method, does:     for (k, v) in E: D[k] = v
            In either case, this is followed by: for k, v in F.items(): D[k] = v
        '''
        if not args:
            raise TypeError("descriptor 'update' of 'MutableMapping' object "
                            "needs an argument")
        self, *args = args
        if len(args) > 1:
            raise TypeError('update expected at most 1 arguments, got %d' %
                            len(args))
        if args:
            other = args[0]
            if isinstance(other, Mapping):
                for key in other:
                    self[key] = other[key]
            elif hasattr(other, "keys"):
                for key in other.keys():
                    self[key] = other[key]
            else:
                for key, value in other:
                    self[key] = value
        for key, value in kwds.items():
            self[key] = value

I can follow it for the most part, and I checked and for some reason that I can't figure I figured out with the help of this SO answer that

issubclass(dict, collections.Mapping) ==> True

I don't quite get why the earlier conditions need to exist… but I'm assuming that has something to do with propagating different kinds of __setitem__ __getitem__ logic in a way that

for key, value in other:
    self[key] = value

fails to capture. But I can't figure out how that would be…

Should I include the somewhat bizarre signature they use? I.e., instead of update(*args, **kwargs) with self, *args = args, I feel like I prefer the more standard update(self, *args, **kwargs) since it is a instance method for NotebookNode.

[minrk]

This should also check and not isinstance(value, NotebookNode) because nodes are dicts, too.

[mpacer]

Shouldn't that be inside from_dict rather than inside here?

[mpacer]

The reasoning for including it in from_dict is that that should be a safety check extant everywhere from_dict is used.

I'm guessing we won't want to change from_dict as used in v1/v2/v3 but I can also propagate the change there.

[minrk]

from_dict always makes a copy, whereas assignment here shouldn't unless necessary. If you want, from_dict could handle that, but I don't think from_dict should be called except on an actual known duct that's not a notebook node.

[minrk]

For safety, now that I think of it, leave from_dict unchanged and just do the check here.

[minrk]

I did not know that update could be called in so many ways!

[minrk]

I'd also be super OK with only accepting .update(self, other) which would be two lines if we assume that other is a Mapping.

[mpacer]

@minrk would you prefer only accepting update(self, other)? I can change it to do that if you'd prefer. I'd like to get this merged so I can move forward on the release.

[minrk]

I think it's AOK since you've already done it. I wouldn't have made you, though, if you hadn't already.

[minrk]

Thanks!