Define __all__ for units#15862
Conversation
Signed-off-by: nstarman <[email protected]>
Signed-off-by: nstarman <[email protected]>
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
|
👋 Thank you for your draft pull request! Do you know that you can use |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
I like the principle very much (plus the simplification of __init__.py), but the implementation less so. Rather than add something somewhat arbitray to def_unit, I think it makes more sense to define a new function that just looks in the namespace and creates a list of all UnitBase instances. Indeed, it barely needs a function, one can just do at the end of each module,
__all__.extend(n for n, v in _globals().items() if isinstance(v, UnitBase))
(though a new function is fine too, which can return an iterator or list)
A bit separately, but a benefit of __all__ is that we don't have to clean up the namespace any more.
nstarman
force-pushed
the
units-import-order
branch
from
f3553b4 to
3253d3e
Compare
nstarman
force-pushed
the
units-import-order
branch
from
3253d3e to
9302516
Compare
nstarman
force-pushed
the
units-import-order
branch
2 times, most recently
from
5802009 to
fc64572
Compare
| __all__ += photometric.__all__ | ||
| __all__ += structured.__all__ | ||
| __all__ += equivalencies.__all__ | ||
| __all__ += function_units.__all__ |
There was a problem hiding this comment.
Why not write it as e.g.:
__all__ = (
module1.__all__ +
module2.__all__ +
...
)
rather than do it via a series of updates?
There was a problem hiding this comment.
See https://github.com/python/typing/blob/main/docs/source/libraries.rst#library-interface-public-and-private-symbols for how __all__ can be updated and make static type checkers happy.
There was a problem hiding this comment.
__all__ += submodule.__all__ is my personal preference in this scenario. We could do __all__.extend(submodule.__all__), but I find it less clean.
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Looking very nice! Mostly comments about the __init__.py, including what I think are missing ones from function.
| from .equivalencies import * | ||
|
|
||
| from .function import units as function_units | ||
| from .function.core import * |
There was a problem hiding this comment.
I guess this and the next one could in principle be replaced by from .function import *, though ideally then we also at an __all__ to its __init__.py. Not sure at all why that one doesn't include a from .units import *! Would be more logical.
(I think there may have been problems before when units.function was also typeset in sphinx; but that is no longer the case.)
There was a problem hiding this comment.
Done! I added from .units import * in units.function and from .function import * in units.
| from . import equivalencies | ||
| from .equivalencies import * | ||
|
|
||
| from .function import units as function_units |
There was a problem hiding this comment.
I had to look back through the git logs to understand why this is imported. It became clear the only reason is to mention it in set_enabled_units below, but of course there we might as well just do function.units, which seems much cleaner. Adjust?
There was a problem hiding this comment.
Though perhaps one should then make sure to add from . import function - which would seem logical anyway.
| __all__ += photometric.__all__ | ||
| __all__ += structured.__all__ | ||
| __all__ += equivalencies.__all__ | ||
| __all__ += function_units.__all__ |
There was a problem hiding this comment.
I think this is missing function.core.__all__ and function.logarithmic.__all__ -- though I think those two and function_units.__all__ could be replaced by function.__all__ if we bring that up to date too.
|
|
||
| core.def_unit(["µas"], u.microarcsecond, doc="microsecond of arc", namespace=_ns) | ||
| core.def_unit(["mas"], u.milliarcsecond, doc="millisecond of arc", namespace=_ns) | ||
| core.def_unit( |
There was a problem hiding this comment.
Ideally, undo these changes (also below).
nstarman
force-pushed
the
units-import-order
branch
from
fc64572 to
23e3914
Compare
Signed-off-by: nstarman <[email protected]>
nstarman
force-pushed
the
units-import-order
branch
from
23e3914 to
cdcaa0d
Compare
|
Changelog needed? |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Super! This looks all OK, now. I don't see why a changelog entry would be needed, since this should not be visible to the user...
4 participants
This PR:
units.__init__.py.def_unitto add the units to__all__if it exists.__all__tounits.Plz squash.