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.

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

LGTM. Thanks!

[mhvk]

I like the bigger of your changes, but note that it actually does not solve the PLR0911 that triggered it, since it needs a noqa now -- which itself decreases code clarity.

The other change makes, in my opinion, the code less clear (though a change like the one you made for the bigger set would improve it).

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.

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.

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. It also have more general doubts about the cost-benefit ratio for making and reviewing these types of PRs, especially if they're done more semi-automatically by novices (here, it is obvious you actually thought about it, so it falls on the positive side for me).

[mhvk]

I think this is an example of a rule forcing worse code style - since every branch has an immediate return, what was there was super clear. Not enough to nix this, but it adds to my feeling that we're starting to apply rules where common sense would be a better guide.

I think a change like you make below, where every branch that has a return has an if instead of an elif would be a better improvement.

[eerovaher]

The one argument in favor of having a single return statement is that all the strings are formatted with :>20 and the single return statement allows not having to repeat that.

[mhvk]

That's a fair argument! As I noted above, I don't feel strongly enough to nix the PR for it, but worried we were letting rules force styles. Given that you made the changes, I'm happy to go with them - would just suggest not to start enforcing the rule (and thus not have noqa for it).

[mhvk]

This noqa just makes the code less clear for most devs who will have no clue what this means.

[mhvk]

This is definitely for the better.

[mhvk]

I like the pattern of if ...: ... return as being very clear that this is a piece of code that just exists.

Might add an empty line in between myself, but really this is nicer than what was there.

[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.