Fix copying of typemap and refactor TypeConfigurator #1302
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1302 +/- ##
==========================================
- Coverage 91.70% 91.63% -0.07%
==========================================
Files 42 42
Lines 9615 9613 -2
Branches 1940 1941 +1
==========================================
- Hits 8817 8809 -8
- Misses 519 522 +3
- Partials 279 282 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| termset_path = os.path.join(CUR_DIR, config_termset_path['termset']) | ||
| termset = TermSet(term_schema_path=termset_path) | ||
| val = TermSetWrapper(value=val, termset=termset) | ||
| return val |
There was a problem hiding this comment.
The changes here
- refactor the logic so that we have fewer nested if/else statements. Return breaks the if so we can dedent
- Updates
configurator.pathtoconfigurator.paths
| self.paths = [] | ||
| if kwargs["paths"]: | ||
| for p in kwargs["paths"]: | ||
| self.load_type_config(p) |
There was a problem hiding this comment.
It was confusing for path to be a list of paths, so change path to paths. It was also confusing for init to take only one path when there may be multiple paths you want to load, and so subsequent calls require calling load_type_config.
| """ | ||
| config_path = kwargs['config_path'] | ||
| type_map = kwargs['type_map'] or get_type_map() | ||
| type_map = kwargs['type_map'] or __TYPE_MAP |
There was a problem hiding this comment.
We want to use the global type map here, not a copy of the global type map. This worked before because copying the global type map maintained a reference to the same type configurator, so the configurator was unintentionally global.
|
I added notes to aid review inline in the PR. |
| # return the value if the data type for the container is not in the config | ||
| if data_type not in config_namespace['data_types']: | ||
| return val |
There was a problem hiding this comment.
I removed the warning here because not all data types used will be configured
| # return the value if the namespace for the container is not in the config | ||
| if self.namespace not in termset_config['namespaces']: | ||
| msg = "%s not found within loaded configuration." % self.namespace | ||
| return val |
There was a problem hiding this comment.
I removed the warning here because not all namespaces used will be configured
|
It is expected that PyNWB tests fail. They only worked because the TypeConfigurator was the same instance across copies and PyNWB was using copies of TypeMap when loading configurations. This is fixed in NeurodataWithoutBorders/pynwb#2121 |
oruebel
left a comment
oruebel
left a comment
There was a problem hiding this comment.
Looks good to me. @stephprince could you also take a look. The TypeMap stuff is pretty deep in the weeds and it would be good to get another pair of eyes on this just to make sure.
|
This also resolves NeurodataWithoutBorders/pynwb#1939 |
Motivation
Fix #1301, work toward fixing NeurodataWithoutBorders/pynwb#1958
Also deprecates calling
get_type_mapwith any 'extensions' arguments, a use case I have never seen. IMO it's easy for the rare user to just callload_namespaceson the type map than us maintain all this extra convenience logic that even we don't use.Checklist
CHANGELOG.mdwith your changes?