Change dtype conversion warning to include path to type #311
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #311 +/- ##
==========================================
+ Coverage 72.76% 72.83% +0.06%
==========================================
Files 33 33
Lines 6443 6452 +9
Branches 1410 1411 +1
==========================================
+ Hits 4688 4699 +11
+ Misses 1327 1326 -1
+ Partials 428 427 -1
Continue to review full report at Codecov.
|
|
I think having the spec path should be helpful enough. At least for developers it should make the debugging easier. |
src/hdmf/spec/spec.py
Outdated
| spec_path = self.name | ||
| spec = self.parent | ||
| while spec is not None: | ||
| spec_path = spec.name + '/' + spec_path |
There was a problem hiding this comment.
I couple things...
- Not all specs have
nameset, so in the case ofdata_typespecs (e.g. DynamicTable), this will be ambiguous. You might consider having the BaseStorageSpec constructor pass indata_type_defasname. - Since parent can't be reassigned, shouldn't this value be constant i.e. it shouldn't need to be computed every time the function gets called?
- I think this should be a property. Is there a reason you didn't go that route?
- I prefer
pathtopath_str. What do you think?
There was a problem hiding this comment.
- Not all specs have
nameset, so in the case ofdata_typespecs (e.g. DynamicTable), this will be ambiguous. You might consider having the BaseStorageSpec constructor pass indata_type_defasname.
Gotcha. It turns out there is already in Validator.get_spec_loc that does what Spec.path_str was trying to do, but takes into account cases with a data_type_def or data_type_inc and no name. So I moved that code into Spec and refactored Validator.get_spec_loc(cls, spec) to call it.
- Since parent can't be reassigned, shouldn't this value be constant i.e. it shouldn't need to be computed every time the function gets called?
Parent can be assigned after the spec is created. It's an odd case, but you can create a GroupSpec A with a path 'A'. Spec A could be added to GroupSpec B and would then have path 'B/A'. Spec B could be added to GroupSpec C and would then have path 'C/B/A', and so on.
- I think this should be a property. Is there a reason you didn't go that route?
No reason. It does make sense as a property (getter only). I changed it. However, if it is not a constant, should it still be a property?
- I prefer
pathtopath_str. What do you think?
Agreed. I like path better. I changed it.
Also add test for no name
…hdmf into enh/convert_dtype_warn
src/hdmf/spec/spec.py
Outdated
| @@ -361,6 +375,24 @@ def resolve_spec(self, **kwargs): | |||
| for attribute in inc_spec.attributes: | |||
| self.__new_attributes.discard(attribute) | |||
| if attribute.name in self.__attributes: | |||
| # copy over the fields 'shape' and 'dims' of the parent attribute to this spec's attribute | |||
There was a problem hiding this comment.
This modification happened in #321
src/hdmf/spec/spec.py
Outdated
| @@ -680,6 +712,30 @@ def __is_sub_dtype(cls, orig, new): | |||
| @docval({'name': 'inc_spec', 'type': 'DatasetSpec', 'doc': 'the data type this specification represents'}) | |||
| def resolve_spec(self, **kwargs): | |||
| inc_spec = getargs('inc_spec', kwargs) | |||
| # copy over fields of the parent dataset to this spec | |||
There was a problem hiding this comment.
This modification happened in #321
Fix #306.
Note that this gives the path to the spec value, NOT the container hierarchy. It might not be as intuitive to users as giving the container hierarchy; however, that would require a large amount of refactoring, given how certain functions are currently encapsulated.