Repository navigation
Cosmo io private - #14842
Cosmo io private#14842
Conversation
|
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 |
91b4da7 to
d8152da
Compare
b8269ad to
f550642
Compare
eerovaher
left a comment
eerovaher
left a comment
There was a problem hiding this comment.
If astropy.cosmology.io is private then changing its name to make it clearer is a good idea. However, this pull request seems to contain unrelated changes and the documentation build failure looks to be related.
eerovaher
left a comment
eerovaher
left a comment
There was a problem hiding this comment.
There are quite a few instances where comments label astropy.cosmology._io as a third party module, which is not true. I have highlighted one of them, but there are more. I don't think the # THIRD PARTY and # LOCAL comments provide much benefit anyways, at least not if we are using Ruff to keep the imports sorted. It would be simplest to just remove these comments.
Furthermore, f70ba0c should be merged with the commit that broke the documentation build.
f70ba0c to
e9f520c
Compare
eerovaher
left a comment
eerovaher
left a comment
There was a problem hiding this comment.
This is mostly good, but I do have a question about an unused import.
e9f520c to
3104de7
Compare
Signed-off-by: Nathaniel Starkman <[email protected]>
Signed-off-by: Nathaniel Starkman <[email protected]> update docs Signed-off-by: Nathaniel Starkman <[email protected]>
3104de7 to
c1fd8c5
Compare
| from astropy.utils.decorators import deprecated | ||
| from astropy.utils.state import ScienceState | ||
|
|
||
| from . import _io # Register I/O methods for reading builtin realizations # noqa: F401 |
There was a problem hiding this comment.
I can't understand what this comment is trying to say. I would imagine that if we are registering methods from _io then we would access them at the moment of registration, but then the module wouldn't be unused and the noqa instruction wouldn't be needed.
There was a problem hiding this comment.
The I/O methods are only registered when the registration code is imported. When trying to read the in-built realizations, it doesn't know about any I/O methods unless _io has been imported first.
There was a problem hiding this comment.
I would like to replace the I/O registration with entry-points, but that will require a PR to io/registry.
There was a problem hiding this comment.
When trying to read the in-built realizations, it doesn't know about any I/O methods unless
_iohas been imported first.
So is it the case that the I/O method registration requires a specific module import order and the line here is meant to enforce that? If so then that's what the comment should say.
There was a problem hiding this comment.
The import order importance is that ._io is registered before .realizations.
Signed-off-by: Nathaniel Starkman <[email protected]>
c1fd8c5 to
3515a4f
Compare
eerovaher
left a comment
eerovaher
left a comment
There was a problem hiding this comment.
Looks good to me, but I'll leave the pull request open for a while in case @WilliamJamieson or @mhvk want to have a look at it.
|
I still don't really like having private modules (unless they are bindings to C code or so) hence felt I should not review this, as really what counts is not my opinion but whether it works better for |
|
I think anybody that would have wanted to say something has had plenty of time to do so, there's no need to delay this any further. |
|
Thanks @eerovaher for the multiple detailed reviews! |
It says in multiple ways that
cosmology.iois private: in its absence in the docs, incosmology.io.__init__.pyand that it's not incosmology.__init__::__all__. This PR helps enforce that.Also, this PR does some small improvements: renaming arguments and cleaning up imports.