Skip to content

Suggestion: simplify constructor for sliceable dicts #10

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

Conversation

matthew-brett
Copy link

I rather like this simplification of the sliceable dict classes, because it
strips them down to only what they are meant to do - allow slicing per element
as well as key access. This PR is to ask you what you think:

Try dropping the special cases for the contructors for sliceable dicts,
and deal with None as input value in the tractogram properties.

@MarcCote
Copy link
Owner

MarcCote commented Jun 7, 2016

I like the simplification too. I don't remember why I was specificly checking if the args[0] was a sliceable dict in the constructor. The tests don't seem to complain. Do you know if self.update(dict(args[0])) does something fancy compared to self.update(args[0])? I do remember that LazyDict needed special care though because of accessing the element of the lazy dict returns a generator instead of the function that creates that generator.

@matthew-brett
Copy link
Author

Looking at the update method of MutableMapping, the first and only positional argument should be something that generates a dictionary, either a dictionary / mutable mapping, or a sequence of tuples. Then all keyword arguments are inserted as keys into the dictionary.

So, the previous code had this for a SliceableDataDict:

self.update(**args[0])

With a single SliceableDataDict as positional arg to `init``, this PR leads to:

self.update(dict(args[0])

I think, because the SliceableDataDict is a mutable mapping, these will lead to the same result - in the original code the key, value pairs get passed as kwargs, in the new code they get passed as the positional argument.

Try dropping the special cases for the contructors for sliceable dicts,
and deal with None as input value in the tractogram properties.
@matthew-brett matthew-brett force-pushed the suggestion-for-per-array-dicts branch from 6b49f33 to f34b779 Compare June 7, 2016 17:51
@MarcCote
Copy link
Owner

MarcCote commented Jun 7, 2016

Great. Is it ready to be merged?

@matthew-brett
Copy link
Author

Yes, ready for merge.

@MarcCote MarcCote merged commit 0324bce into MarcCote:streamlines_api Jun 7, 2016
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.

2 participants