Address Ruff rule PLR0911 in `io`

[eerovaher]

Description

PLR0911 guards against functions with very many return statements. The three functions in astropy.io violating that rule (which all happened to be in io.fits) have been updated. Two of them no longer violate the rule. The third has fewer return statements than before but still needs a noqa instruction. The updated code also has fewer violations of rules C901, SIM108, SIM114, PLR0912 and RET505.

I was able to dedent a lot of code in _convert_to_valid_data_type(), so for reviewing the changes it might be helpful to tell Git to ignore lines where the only change was in their indentation level:

$ git diff --stat main 
 .ruff.toml                     |   1 -
 astropy/io/fits/card.py        |  26 +++++----------
 astropy/io/fits/column.py      | 115 ++++++++++++++++++++++++++++------------------------------------
 astropy/io/fits/hdu/hdulist.py |   8 ++---
 4 files changed, 62 insertions(+), 88 deletions(-)
$ git diff --stat --ignore-space-change main 
 .ruff.toml                     |  1 -
 astropy/io/fits/card.py        | 20 ++++++--------------
 astropy/io/fits/column.py      | 33 +++++++++------------------------
 astropy/io/fits/hdu/hdulist.py |  8 +++-----
 4 files changed, 18 insertions(+), 44 deletions(-)

This option is also available in the GitHub UI.

• [x] 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[bot]]

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?
• [ ] 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.

[nstarman]

I think instead we might start a collection of rules that we agree not to enforce but which perhaps sometimes should be rechecked, so see whether code can be made clearer.

I like this idea! We can make a section in the pyproject.toml separated from the permanent ignores by a comment.

More generally, I very strongly feel that not every rule that is included in ruff has to be followed -- it is not obvious all "rules" are good ones.

I agree. That's why we have the pyproject.toml permanent ignores. If we don't want to discover organically as Issues/PRs are made which rules we like, we could have a hack day to triage? The ruff.toml exists entirely as the holding file for triaging. We've had a number of good PRs addressing much of the easy opportunities to improve the code. Now, we've most certainly reached the tipping point of the 80/20, where we're now in the last 20% of code improvement and clarity, but 80% of the work (e.g. #16060 is a good high effort PR, but offers the opportunity to improve our treatment of file paths and more uniformly support Python standards). It's AOK if we want to change our strategy going forward. I think a hack day could be an easy way to assess the majority of what remains in ruff.toml.

[eerovaher]

Replying to @mhvk

So, my suggestion would be make some changes, but not to start enforcing this rule, as it seems clear that it is useful only as a hint that something may be bad, rather than as one that something is bad.

Ruff has a few rules that count something in a function and complain if the count exceeds some threshold value (e.g. C901, PLR0911, PLR0912, PLR0915). The threshold values are inevitably somewhat arbitrary, so I fully agree with your interpretation of what the rule violations mean. Where our opinions differ is that I don't think it's distracting to add noqa instructions where these violations are justified.

I think instead we might start a collection of rules that we agree not to enforce but which perhaps sometimes should be rechecked, so see whether code can be made clearer.

Good idea. Consider opening a separate issue for this.

Replying to @nstarman

We've had a number of good PRs addressing much of the easy opportunities to improve the code. Now, we've most certainly reached the tipping point of the 80/20, where we're now in the last 20% of code improvement and clarity, but 80% of the work

There are still many obviously good rules that are simple to implement, even for first time contributors: all of RET, all of SIM, B007, EXE002, PLE0101, PT022 etc.

[github-actions[bot]]

Hi humans :wave: - 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.

[github-actions[bot]]

I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks!

If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here.