Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature]: use __all__ for modules #1686

Open
3 tasks done
bendichter opened this issue Apr 11, 2023 · 4 comments · May be fixed by #2021
Open
3 tasks done

[Feature]: use __all__ for modules #1686

bendichter opened this issue Apr 11, 2023 · 4 comments · May be fixed by #2021
Labels
category: enhancement improvements of code or code behavior help wanted: good first issue request for community contributions that are good for new contributors priority: medium non-critical problem and/or affecting only a small set of NWB users
Milestone

Comments

@bendichter
Copy link
Contributor

What would you like to see added to PyNWB?

Python allows you to use __all__ to indicate the public interface of a module. This is commonly used for the from x import * pattern, where it limits the returned classes to those listed in the __all__ list.

While I don't necessarily recommend using that import pattern, I still think __all__ would be useful, because it informs the user what public classes are defined in that module. I have a current situation where I would like to import all of the neurodata types implemented in a module. Simply using the dir() command, I get all of the neurodata types that are imported into the module, as well as a bunch of double underscore functions, which I do not want.

from pynwb import ecephys
dir(ecephys)
['CORE_NAMESPACE',
 'ClusterWaveforms',
 'Clustering',
 'DataChunkIterator',
 'Device',
 'DynamicTableRegion',
 'ElectricalSeries',
 'ElectrodeGroup',
 'EventDetection',
 'EventWaveform',
 'FeatureExtraction',
 'FilteredEphys',
 'Iterable',
 'LFP',
 'MultiContainerInterface',
 'NWBContainer',
 'NWBDataInterface',
 'SpikeEventSeries',
 'TimeSeries',
 '__builtins__',
 '__cached__',
 '__doc__',
 '__file__',
 '__loader__',
 '__name__',
 '__package__',
 '__spec__',
 'assertEqualShape',
 'docval',
 'get_data_shape',
 'get_docval',
 'popargs',
 'popargs_to_dict',
 'register_class',
 'warnings']

__all__ lists are used on many popular packages, such as sci-kit learn: https://github.com/scikit-learn/scikit-learn/blob/main/sklearn/feature_extraction/image.py

The __all__ list could be used in a number of ways, for instance to determine coverage of tests, mock classes, and tutorials.

Is your feature request related to a problem?

No response

What solution would you like?

see above

Do you have any interest in helping implement the feature?

Yes.

Code of Conduct

@CodyCBakerPhD
Copy link
Collaborator

I will point out that a part of the reason it works really well on SKlearn is because they also have the __init__ for each sub-module well-partitioned throughout the project

We could (and probably should) do that here as well, where instead of having a single file called ecephys.py you have a folder ecephys with it's own __init__ and any number of sub-files to break up the class definitions in a more readable/searchable manner (rather than scrolling through 351 lines to find the class I'm looking for)

Also note that this entire discussion might sound like it breaks back-compatibility, but if done well (and tested well; related to #1493) it actually wouldn't even be noticeable except that auto-completion of imports contains much less noise

@bendichter
Copy link
Contributor Author

@CodyCBakerPhD I think breaking modules into submodules could be a separable issue. Would you mind if we discussed that in a different thread?

@oruebel
Copy link
Contributor

oruebel commented Apr 11, 2023

Defining __all__ sounds good to me.

I think breaking modules into submodules could be a separable issue

I agree that creating submodules should be a separate issue. I think what is being proposed here, is to have __all__ defined in ecephys.py directly (as well as all the other files, e.g., icephys.py, ophys.py etc.).

@bendichter
Copy link
Contributor Author

@oruebel yes, that's right.

@oruebel oruebel added category: enhancement improvements of code or code behavior priority: medium non-critical problem and/or affecting only a small set of NWB users labels Apr 11, 2023
@oruebel oruebel added this to the Next Release milestone Apr 11, 2023
@bendichter bendichter added the help wanted: good first issue request for community contributions that are good for new contributors label Apr 11, 2023
@stephprince stephprince modified the milestones: 2.7.0, 2.8.0 May 9, 2024
@stephprince stephprince modified the milestones: 2.8.0, 2.9.0 Jul 23, 2024
@rly rly modified the milestones: 2.9.0, Next Major Release - 3.0 Sep 5, 2024
@bendichter bendichter linked a pull request Jan 27, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: enhancement improvements of code or code behavior help wanted: good first issue request for community contributions that are good for new contributors priority: medium non-critical problem and/or affecting only a small set of NWB users
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants