Repository navigation
How to indicate which sub-sub-modules are private? #15169
Description
What is question we should resolve?
Some of the eco-system (e.g., scipy, with numpy set to follow suit) is moving to using names starting with _ to mark any module that only holds "private" functions/classes, which are imported higher up.
In astropy, we have generally not done that (see "additional context" below) but there are now PRs for cosmology (#14982, #15126) and a new utils/metadata (#15166) that are starting to use that system.
I think we need to decide for the package as a whole whether this is a good idea. Advantages:
- Consistency with scipy and numpy (though at least numpy has been a true mess in how their code was organized, mostly for historical reasons of being a merger of numeric and numarray, so they have a reason for reorganizing regardless).
- Clarity for users what is public and private.
Disadvantages:
- Why change what works: Astropy has a clear system where one imports from the highest level something is available (usually just the subpackage, or the submodule of
astropy.utils). We have had very few problems arising from our current system. - Lots of churn if we do this package-wide, needing review time that may be better spent looking at actual code (and I feel we have been dealing with a lot of churn already...).
- Unless done package wide, we will get a mixed state, which will be more confusing for users: for those subpackages which do not follow the
_means private, are those other files meant to be public? How is a user supposed to know?
I think the last disadvantage requires particular care: the confusion resulting from changing some sub-packages will affect mostly those subpackages that do not change (though again, historically we've had very few problems, so possibly this will remain a minor issue).
Personally, I prefer to stick with the system we have, but I am happy to follow majority opinion if a change is deemed for the better. However, I feel strongly that we should be consistent in astropy (and write up the result). Hence, I think we should not merge changes in naming convention until we have had this discussion.
p.s. Somewhat related is what to do with private functions/classes/methods. If we go to private modules, there would seem to be no reason for private functions/classes any more, since anything not exported is by definition now private.
p.s.2 Also related but probably irrelevant: sphinx allows 2 places for API links: the place a function/class is defined, and a place where it should normally be imported from. Having the site of definition private may make things more or less clear.
Describe the desired outcome
A documented decision on whether to stick with our existing convention of what module are private, or more to indicating it by pre-pending underscores to their names. (Plus one on what functions/classes should be marked in addition.)
Unless there is obvious agreement by most but me that we should go with the "prepend _" model, it may be best for the proponents to write an APE.
Additional context
Current set of files:
$ find astropy -name _[^_]\*.py
astropy/cosmology/parameter/_core.py
astropy/cosmology/parameter/_converter.py
astropy/cosmology/_utils.py
astropy/nddata/_testing.py
astropy/timeseries/periodograms/lombscargle/_statistics.py
I.e., beyond cosmology, we have only 2 files:
nddata._testingis only used innddata/testsandnddata.mixins.testsand might more logically live in the former directory. It is really obviously not part of the general sub-package.timeseries/periodograms/lombscargle/_statistics.pycontains functions that are used inside methods of theLombScargleclass. It is unclear to me why this one has a_andcore.pyandutils.pyhave not, though_statistics.pyare arguably less stand-alone. Anyway, arguably an oversight.
EDIT: I should have done my search better:
$ find astropy -name _[^_]\*
astropy/stats/_stats.pyx
astropy/cosmology/parameter/_core.py
astropy/cosmology/parameter/_converter.py
astropy/cosmology/_utils.py
astropy/cosmology/_io
astropy/io/fits/_utils.pyx
astropy/io/fits/_tiled_compression
astropy/convolution/_convolve.pyx
astropy/nddata/_testing.py
astropy/table/_column_mixins.pyx
astropy/table/_np_utils.pyx
astropy/timeseries/periodograms/bls/_impl.pyx
astropy/timeseries/periodograms/lombscargle/_statistics.py
So, there is already one module in cosmology and one in io.fits. The rest is cpython, which makes sense to do differently.
Activity
nstarman commentedon Aug 14, 2023
Very relevant is an APE on this topic astropy/astropy-APEs#85.
nstarman commentedon Aug 14, 2023
Perhaps this is best discussed in the APE, but to the mentioned disadvantages
It only works in one sense, that the highest level something generally has the desired import.
In another important sense, what we have doesn't work: public-looking-but-private modules are also perfectly valid places from which to import things. In the docs we mention in passing that stuff should be imported from the top-level, but that's hardly enough. Per the zen of python "There should be one-- and preferably only one --obvious way to do it".
Perhaps. I don't think that style is so extricable from its content. Having clear and consistent API helps users and maintainers. So I'm not sure I'd categorize this as churn, so much as improving code clarity or addressing technical debt.
On other sources of recent "churn", I think a good rule of thumb is that code should change at a pace similar to the language itself and community standards. IMO much of the recent PRs focusing on code refactoring and style are precisely because the language and standards have changed. From simple improvements like catching repeat keys in dictionaries to more complex endeavours like
blackand ongoing works like usingdataclasses, this is a mix of catch up and keeping pace.I agree, let's do this package-wide.
mhvk commentedon Aug 14, 2023
Sorry, I should definitely have been aware of APE 22!