pynwb icon indicating copy to clipboard operation
pynwb copied to clipboard

Improve package modularity

Open CodyCBakerPhD opened this issue 2 years ago • 4 comments

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.

CodyCBakerPhD avatar Jul 04 '22 18:07 CodyCBakerPhD

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.

bendichter avatar Jul 07 '22 13:07 bendichter

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.

CodyCBakerPhD avatar Jul 07 '22 13:07 CodyCBakerPhD

Codecov Report

Merging #1493 (d8fe134) into dev (2bf5d19) will increase coverage by 0.60%. The diff coverage is 83.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.

codecov[bot] avatar Jul 07 '22 13:07 codecov[bot]

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 ?

CodyCBakerPhD avatar Jul 07 '22 13:07 CodyCBakerPhD