STY: move RET505 and RET506 to permanently ignored rules

[neutrinoceros]

Description

Following this morning's breakout session where we removed these rules from https://github.com/astropy/astropy/issues/14818, we seemed to agree that these are a matter of taste and enforcing them everywhere isn't worth reviewer time, so moving them to permanently ignored rules let contributors focus on stuff we value more.

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

[github-actions[bot]]

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

[nstarman]

Ok by me. My personal preference is to let Ruff auto-fix RET505/6 and be more Zen ("Flat is better than nested"), but it's a small nest and only a little unsightly.

[eerovaher]

I had a look at the code that violates RET505 (superfluous-else-return) by running $ ruff check --select RET505 --config pyproject.toml --output-format full astropy. There are many cases where addressing the rule would be an improvement. There's code that could be replaced with return ... if ... else .. one-liners, e.g. https://github.com/astropy/astropy/blob/d5bd585625baa64c91ca76ae754613ce04b81e34/astropy/wcs/wcsapi/low_level_api.py#L240-L243 could easily be replaced with

        return None if self.pixel_shape is None else self.pixel_shape[::-1]

There's what looks like badly ported C code (Pythonic code would use elif instead of else with a nested if to reduce code indentation): https://github.com/astropy/astropy/blob/d5bd585625baa64c91ca76ae754613ce04b81e34/astropy/wcs/wcsapi/wrappers/sliced_wcs.py#L81-L87 There's code where the indentation levels of large chunks of code could be reduced, e.g. https://github.com/astropy/astropy/blob/d5bd585625baa64c91ca76ae754613ce04b81e34/astropy/visualization/wcsaxes/utils.py#L22-L48 could be rewritten as

    if dv <= 1.0 * u.arcsec:
        return select_step_scalar(dv.to_value(u.arcsec)) * u.arcsec

    degree_limits_ = [1.5, 3, 7, 13, 20, 40, 70, 120, 270, 520]
    degree_steps_ = [1, 2, 5, 10, 15, 30, 45, 90, 180, 360]
    degree_units = [u.degree] * len(degree_steps_)
  
    minsec_limits_ = [1.5, 2.5, 3.5, 8, 11, 18, 25, 45]
    minsec_steps_ = [1, 2, 3, 5, 10, 15, 20, 30]
 
    minute_limits_ = np.array(minsec_limits_) / 60.0
    minute_units = [u.arcmin] * len(minute_limits_)
  
    second_limits_ = np.array(minsec_limits_) / 3600.0
    second_units = [u.arcsec] * len(second_limits_)
  
    degree_limits = np.concatenate([second_limits_, minute_limits_, degree_limits_])
  
    degree_steps = minsec_steps_ + minsec_steps_ + degree_steps_
    degree_units = second_units + minute_units + degree_units
  
    n = degree_limits.searchsorted(dv.to(u.degree))
    step = degree_steps[n]
    unit = degree_units[n]

    return step * unit

In some cases the reduction of indentation level can lead to further code shortening because some lines might then fit inside the 88 character limit instead of having to be split over multiple lines.

In summary it is clear to me that there are many cases where we would benefit from enforcing the rule, but it is less clear if the rule should be enforced in all cases. I wont block merging this, but personally I wouldn't mind enforcing these rules everywhere.

[mhvk]

Nothing about putting any codes on the exclude list prevents anyone from checking what problems exist and making the improvements mentioned! The question is whether a rule should be enforced. For these ones, I felt the conclusion was that "breaking" them is sometimes/often not an improvement, and having to put a noqa: XXX is more trouble than its worth, both in reducing legibility (why should someone reading the code be distracted by it) and in cost of time spent reviewing changes that need to be argued over. I.e., enforcing these two has an opportunity cost that is greater than any benefits.

[nstarman]

@mhvk highlighted that maintainer effort is the primary driver behind permanently ignoring these. But also some stylistic issues. As in if

if boolean:
    return a
return b

is better/equal/worse than

if boolean:
    return a
else:
    return b

My opinion is yes, marginally, because you can see structurally the code never returns None without even having to read the else is not an elif. You can grok a function's I/O at the tab level, even in a code minimap. (Also, ruff would fix this up to a ternary). Small brain speedup applied frequently is a large time-saver. IMO in my code ret505 hasn't ever produced worse code, only equal or better, so I don't have any noqa.

If Ruff could always just auto-fix this for us, then the only burden on maintainers would be in higher-level code layout questions, like @eerovaher has highlighted, which is where we prefer maintainer's time to be spent. Unfortunately the auto-fix option for RET505/6 is not always available ("Fix is sometimes available." on https://docs.astral.sh/ruff/rules/superfluous-else-return/) so Ruff can't always take care of this for us. Why don't we add this to the permanent ignore, indicating we don't need to discuss it in PR reviews, but also add a note that these rules should be revisited once ruff has improved it's auto-fix capabilities. Then there should be no maintainer burden!

[mhvk]

For me, the second example is actually easier - the first reads as if the end will apply to the if part too (i.e., as if it is a continuation; I need to see the return to know that is not true). So, part of the problem is that the extent to which this rule is better is pretty subjective, and it depends on whether "boolean" is something obviously indicating a split. In that respect, I do like @eerovaher's replacement with a ternary - then it is obvious we're dealing with what exactly will get returned.

My sense would be to add a comment that this is a rule that sometimes can help find places where refactoring is very helpful.

[neutrinoceros]

I also think that there is no objectively "better" or "worse" style here, and my personal opinion is that

if condition:
    ... # 1
    return x
else:
    ... # 2
    return y

is generally more readable than

if condition:
    ... # 1
    return x
... # 2
return y

except #1 is very short and #2 isn't, where I tend to prefer the latter style. Anyway, the point of this PR is actually to throw a bone to those of us who experience linting fatigue, so I'd suggest to not bike-shed on it too much: as Marten pointed out, marking these rules as unenforcable doesn't prevent anyone from using their preferred style. I'll add the comment suggested by @mhvk now

[pllim]

Looks like @eerovaher had concerns, so do we need a second approval?

[mhvk]

OK, let's cut the know here, merging.