pynwb
pynwb copied to clipboard
Improve package modularity
Motivation
It can be ambiguous to have callable methods in the __init__
of a package whenever those methods also share a name with a submodule exposed at the same level. E.g., currently pynwb.validate
is both a submodule with methods you can import from and a method intended to be called on an open io
object.
It also makes the package a bit harder to navigate, as it is not intuitive where certain functions might be found, such as namespace-related items (more properly belonging to the spec
module?) and IO functions (like NWBHDF5IO
, should perhaps be in the io
submodule?). It's also not a very common practice to include more than simple import routing, logging, or necessary package-level initialization procedures within the __init__.py
files.
This PR moves all the actual code bits from __init__
to a new module called tools
, however, the global import structure should remain unchanged. I understand this is the most likely point to cause concern for this type of PR, which is why I'm explicitly testing this structure: https://github.com/NeurodataWithoutBorders/pynwb/blob/modularize_init_1/tests/back_compat/test_imports.py (though I can certainly add deeper orders of nesting to that).
This is just a first draft suggestion, I'm open to discussion of where each particular item should go and/or what structure would be most convenient for everyone. Also open to not doing this at all, but I strongly recommend it and it will make some subsequent PR's I have in the works for improvements to the validate
functionality a lot easier to set up.
How to test the behavior?
A new test/class is added to the test_backcompat
set of tests that checks the import structure matches, at least up to newly introduced exceptions.
Checklist
- [ ] Did you update CHANGELOG.md with your changes?
- [X] Have you checked our Contributing document?
- [X] Have you ensured the PR clearly describes the problem and the solution?
- [X] Is your contribution compliant with our coding style? This can be checked running
flake8
from the source directory. - [X] Have you checked to ensure that there aren't other open Pull Requests for the same change?
- [X] Have you included the relevant issue number using "Fix #XXX" notation where XXX is the issue number? By including "Fix #XXX" you allow GitHub to close issue #XXX when the PR is merged.
I agree I'm not a fan of the inits being so big, but I don't know if moving all of these functions to tools
is the best approach. I think a better approach would be to take it module-by-module and see if we can find the most appropriate place for each function.
I agree I'm not a fan of the inits being so big, but I don't know if moving all of these functions to tools is the best approach. I think a better approach would be to take it module-by-module and see if we can find the most appropriate place for each function.
Absolutely! The tools
module is just a temporary home and proof of concept, we can go through each function one by one and decide the best place for them.
Codecov Report
Merging #1493 (d8fe134) into dev (2bf5d19) will increase coverage by
0.60%
. The diff coverage is83.06%
.
@@ Coverage Diff @@
## dev #1493 +/- ##
==========================================
+ Coverage 78.42% 79.02% +0.60%
==========================================
Files 37 39 +2
Lines 2777 2804 +27
Branches 517 517
==========================================
+ Hits 2178 2216 +38
+ Misses 518 507 -11
Partials 81 81
Impacted Files | Coverage Δ | |
---|---|---|
src/pynwb/tools.py | 68.00% <68.00%> (ø) |
|
src/pynwb/__init__.py | 100.00% <100.00%> (+23.35%) |
:arrow_up: |
src/pynwb/base.py | 100.00% <100.00%> (ø) |
|
src/pynwb/behavior.py | 100.00% <100.00%> (ø) |
|
src/pynwb/core.py | 66.26% <100.00%> (+0.41%) |
:arrow_up: |
src/pynwb/device.py | 100.00% <100.00%> (ø) |
|
src/pynwb/ecephys.py | 96.42% <100.00%> (+0.03%) |
:arrow_up: |
src/pynwb/epoch.py | 88.67% <100.00%> (+0.21%) |
:arrow_up: |
src/pynwb/file.py | 87.71% <100.00%> (+0.03%) |
:arrow_up: |
src/pynwb/globals.py | 100.00% <100.00%> (ø) |
|
... and 18 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 2bf5d19...d8fe134. Read the comment docs.
List of method/variabless and current proposed destinations. ???
indicate I'm unsure where the best place might be.
`` Globals
__resources
-> spec? (followed by loading of namespaces from the resources)
__NS_CATALOG
-> spec
__TYPE_MAP
-> spec
hdmf_typemap
(not capitalized?) -> spec
Last two here are purely hand-coded strings
CORE_NAMESPACE
-> globals
__core_ns_file_name
-> globals
Methods
__get_resources
-> spec
?
_get_resources
-> legacy
get_type_map
-> spec
?
get_manager
-> spec
?
load_namespaces
-> spec
available_namespaces
-> spec
register_class
-> registration
?
register_map
-> registration
?
get_class
-> ???
validate
-> validation
NWBHDF5IO
-> io
export
-> io
?