Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #87 +/- ##
==========================================
+ Coverage 95.57% 95.71% +0.13%
==========================================
Files 11 12 +1
Lines 1604 1702 +98
==========================================
+ Hits 1533 1629 +96
- Misses 71 73 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| name_validation=None, plugins=None): | ||
| self._open_datasets = weakref.WeakValueDictionary({}) | ||
| directory = pathlib.Path(directory) #.resolve() | ||
| directory = pathlib.Path(directory).absolute() #.resolve() |
There was a problem hiding this comment.
Why is this necessary? Not that I mind, just good to know why it was added.
dragly
left a comment
dragly
left a comment
There was a problem hiding this comment.
The fundamental parts look really good! I would just change some of the behavior :)
| return self | ||
| return super(File, self).__getitem__(path) | ||
|
|
||
| def __setitem__(self, name, value): |
There was a problem hiding this comment.
Is this related to links or is it just something we have left out from before? Seems like this is something that has been missing on File regardless of links?
|
|
||
| def _link(self, name, get_link=False): | ||
| link_meta = self._group(name).meta[exob.EXDIR_METANAME][exob.LINK_METANAME] | ||
| print(link_meta) |
| EXDIR_METANAME = "exdir" | ||
| TYPE_METANAME = "type" | ||
| VERSION_METANAME = "version" | ||
| LINK_METANAME = "link" |
There was a problem hiding this comment.
I think I prefer if the type is soft_link, external_link, etc. That reduces the level of conditional nesting - you can say if type == "external_link" instead of if type == "link" and link_type == "external". And then have the meta-groups be soft_link and external_link as well. It just reduces the confusion of which variables can go together. (No-one will think that they can just change the target from external to soft and still keep the file, for instance.
| f = setup_teardown_file[3] | ||
| g = File(external_path, 'w') | ||
| f['ext'] = ExternalLink(external_path, '/missing') | ||
| with pytest.raises(KeyError): |
There was a problem hiding this comment.
To add to the comment above: This should not be a KeyError. It should actually return the object that the link is pointing to.
| return len([a for a in self]) | ||
|
|
||
| def get(self, key): | ||
| def get(self, name, get_link=False): |
There was a problem hiding this comment.
I think the getter should first try to see if it is an object or a link, instead of forcing the user to tell it to expect a link or not. This is also how h5py works.
2 participants
Should fix #1
Aims for compatibility with NWB NeurodataWithoutBorders/pynwb#629