restore public status of NWBMetaDataEncoder encoder
#1142
Conversation
| This encoder extends the default JSONEncoder class and provides custom serialization | ||
| for certain data types commonly used in NWB metadata. | ||
| """ | ||
| class _GenericNeuroconvEncoder(json.JSONEncoder): |
There was a problem hiding this comment.
@bendichter here.
|
|
||
| return super().default(obj) | ||
| # This is used in the Guide so we will keep it public. | ||
| NWBMetaDataEncoder = _NWBMetaDataEncoder |
There was a problem hiding this comment.
I think this is a strange pattern. Why not just define it as public to begin with?
There was a problem hiding this comment.
I would like to keep things private by default in general. Then we don't have to worry when we refactor, move stuff, change signature, etc. It is less work for us for something that is not our core service and it frees resources for more important stuff.
The problem here is that Ryan was using that one, then we made it private at some point in June which broke something on its end. We are making it public for him here
There was a problem hiding this comment.
See the issue here:
#1141
Magic was used to solve the compatibility break from path and strings to pydantic with aliases and then that magic broke something else. This is restoring / addressing this.
| class _NWBSourceDataEncoder(_NWBMetaDataEncoder): | ||
| class _NWBMetaDataEncoder(_GenericNeuroconvEncoder): | ||
| """ | ||
| Custom JSON encoder for data interface source data (i.e. kwargs). | ||
|
|
||
| This encoder extends the default JSONEncoder class and provides custom serialization | ||
| for certain data types commonly used in interface source data. | ||
| Custom JSON encoder for NWB metadata. | ||
| """ | ||
|
|
||
| def default(self, obj): | ||
|
|
||
| # Over-write behaviors for Paths | ||
| if isinstance(obj, Path): | ||
| return str(obj) | ||
|
|
||
| return super().default(obj) | ||
| class _NWBSourceDataEncoder(_GenericNeuroconvEncoder): | ||
| """ | ||
| Custom JSON encoder for data interface source data (i.e. kwargs). | ||
| """ | ||
|
|
||
|
|
||
| class _NWBConversionOptionsEncoder(_NWBMetaDataEncoder): | ||
| class _NWBConversionOptionsEncoder(_GenericNeuroconvEncoder): | ||
| """ |
There was a problem hiding this comment.
I think this is a strange pattern. Why define child classes that are identical to the parent class? Why not just use the parent class?
There was a problem hiding this comment.
They might diverge and we might not want the same behavior for each of those cases.
There was a problem hiding this comment.
Given that they are private though, I don't mind having the simple case as you want it and then refactoring if and when we need it.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1142 +/- ##
==========================================
+ Coverage 90.69% 90.88% +0.18%
==========================================
Files 129 129
Lines 8189 8205 +16
==========================================
+ Hits 7427 7457 +30
+ Misses 762 748 -14
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This should fix #1141.
I want to do this after #1139 to minimize conflict errors. This PR has #1139 as base.