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

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

Merged
merged 8 commits into from
Aug 18, 2017
37 changes: 32 additions & 5 deletions nbformat/notebooknode.py
Original file line number Diff line number Diff line change
@@ -1,23 +1,50 @@
"""NotebookNode - adding attribute access to dicts"""

from ipython_genutils.ipstruct import Struct
from collections import Mapping


class NotebookNode(Struct):
"""A dict-like node with attribute-access"""
pass

def __setitem__(self, key, value):
if isinstance(value, Mapping):
value = from_dict(value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

super(NotebookNode, self).__setitem__(key, value)

def update(self, *args, **kwargs):
"""
A dict-like update method based on CPython's MutableMapping `update`
method.
"""
if len(args) > 1:
raise TypeError('update expected at most 1 arguments, got %d' %
len(args))
if args:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 kwargs.items():
self[key] = value


def from_dict(d):
"""Convert dict to dict-like NotebookNode

Recursively converts any dict in the container to a NotebookNode.
This does not check that the contents of the dictionary make a valid
notebook or part of a notebook.
"""
if isinstance(d, dict):
return NotebookNode({k:from_dict(v) for k,v in d.items()})
return NotebookNode({k: from_dict(v) for k, v in d.items()})
elif isinstance(d, (tuple, list)):
return [from_dict(i) for i in d]
else:
return d