Add missing dunder __all__ to star-imported modules

[alchzh]

Description

This pull request is to address a couple of modules (astropy.io.fits.hdu.compressed and astropy.visualization) that currently have unintended exports because of star-imported modules without __all__ defined.

Removes the following exports from astropy.visualization: {'np'}
Removes the following exports from astropy.io.fits.hdu.compressed: {'lazyproperty', 'compress_image_data', 'ctypes', 'DTYPE2BITPIX', 'math', 'BinTableHDU', 'DELAYED', 'BITPIX2DTYPE', 'deprecated_renamed_argument', 'np', 'time', 'FITS_rec', 'ExtensionHDU', 'conf', 'simplify_basic_index', 'suppress', 'decompress_image_data_section', 'ImageHDU'}

Represents a breaking change, though I hope no one is accessing NumPy as astropy.visualization.np or ctypes as astropy.io.fits.hdu.compressed.ctypes. Apart from external modules or names exported correctly at a different location, this makes compress_image_data and decompress_image_data_section "private". I don't think they're supposed to be accessible on astropy.io.fits.hdu.compressed since they're defined in _tiled_compression.py, but pinging @astrofrog to check.

• By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

[github-actions]

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.

• Do the proposed changes actually accomplish desired goals?
• Do the proposed changes follow the Astropy coding guidelines?
• Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
• Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
• Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
• Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
• Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
• Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
• Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
• At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

[pllim]

Hmm, @nstarman or @eerovaher , did you set up the ruff check for this stuff? I vaguely remember a bunch of discussions about star imports. So either this is unnecessary or those checks are not working?

@alchzh , if this PR were to move forward, I think it needs 2 change logs because it touches two completely unrelated subpackages. Or better yet, 2 different PRs. But let's wait to hear back first. Thanks!

[alchzh]

I don't think Ruff supports checking if __all__ is missing or present (astral-sh/ruff#3482). I found these cases by running my lazy_loader transformer which inspects __all__ on any star imports.

[eerovaher]

Ruff has a rule about using import * and it has rules that check the format and contents of __all__ if it is defined. Those rules are working as intended. But (like @alchzh already pointed out) Ruff doesn't have a rule that would force __all__ to be defined. So what is happening is that because visualization.__init__.py contains from visualization.units import * (which is allowed because it is a __init__.py file) but there is no visualization.units.__all__ then everything in visualization.units not beginning with an underscore becomes available from visualization, and apparently that includes np.

[nstarman]

did you set up the ruff check for this stuff? I vaguely remember a bunch of discussions about star imports. So either this is unnecessary or those checks are not working?

As suggested in APE 22 we can make a pre-commit hook that checks for __all__ and checks that each thing in __all__ has a corresponding doc in docs/api. That would catch all these cases. This hook does not yet exist. I think the check for the existence of __all__ should be straightforward as a file-scoped regex.

[nstarman]

Thanks @alchzh. It's great to further clarify our API! Aside from the changelog, which @pllim commented on, this is looking good.

[alchzh]

I've split the changelog into two fragments in this PR.

[pllim]

Thanks! I think the diff technically looks good now but we have to wait for io.fits and visualization maintainers to okay this (see https://www.astropy.org/team for more info). Given the holidays, replies might be delayed.

Happy holidays!

[alchzh]

Sounds great. I hope that all devs are enjoying the holidays more than bored student with nothing to do after the semester ends 🤣

Happy Holidays!

[eerovaher]

According to PEP8 __all__ should be placed after the module docstring, but before the imports, also in the other files.

[nstarman]

Except
For __future__ imports, which precede everything but the module docstring.

[alchzh]

I tried to follow the convention in the rest of the library: 232 files define __all__ after the imports (excluding from __future__ ... while only 22 define __all__ before imports by my count. This is one of the PEP8 suggestions that no one know about or bother to follow if they do:

• none of the style checkers/formatters (pycodestyle, black, ruff, isort, etc.) support it, not even with extensions
• numpy, scipy, pandas, and probably many more scientific libraries put __all__ after imports

So I figured in this case it's better to be consistent than follow an unenforced guideline. But I'll defer to you all if you feel strongly about sticking to PEP8.

[eerovaher]

Most of astropy places __all__ below the imports and changing that is not a high priority, but I don't see a good reason why brand new __all__ definitions should knowingly violate PEP8.

[nstarman]

We should open an issue on Ruff's isort to enable sorting __all__ to be above the imports.

[alchzh]

PEP8 tells us:

A style guide is about consistency. Consistency with this style guide is important. Consistency within a project is more important. Consistency within one module or function is the most important.

All other files in io and visualizations and 94% of all astropy files (with __all__) place __all__ after imports, so unless there's a explicit directive to change the style in all new code I think doing it just for this PR violates both consistency within one module and consistency within a project.

[nstarman]

astral-sh/ruff#9324

[nstarman]

@eerovaher I suggest that since this discussion affects Astropy package-wide we should have it post getting this in.

[nstarman]

Let's get this in. We should come up with our __all__ location policy in a followup PR or something.

[github-actions]

Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

[nstarman]

@pllim can we merge this or does it need a rebase?

[neutrinoceros]

seeing the amount of now-required jobs that never ran here I would recommend a rebase

[nstarman]

I can't trigger a re-base nor a re-run. We should add manual running to the tests CI for the latter.

[pllim]

I can't trigger a re-base

Hmm, not sure what you mean here, you should be able to rebase and force push to author's branch here as a core maintainer.

[nstarman]

In other repos I have enabled

[pllim]

You can already do this. But if you don't want to, please let me know and I can do it for you. Thanks. Probably not the coolest way but it works.

git remote add alchzh git@github.com:alchzh/astropy.git
git fetch alchzh add-dunder-all-to-star-imports
git checkout alchzh/add-dunder-all-to-star-imports -b pr15781
git fetch upstream main
git rebase upstream/main
git push alchzh pr15781:add-dunder-all-to-star-imports -f

[pllim]

tsk tsk, doc is currently failing from unrelated issue, but I want to make sure doc is green in this PR because __all__ does affect API docs, so I think we have to wait more (#16481 or #16482).

[pllim]

Would you mind rebasing again to make sure RTD builds? If not, I can do it. Please let me know. Thanks for your patience!

[pllim]

Thanks, all!