Ensure compliance with more Ruff rules

[eerovaher]

What is the problem this feature will solve?

Ruff is a Python linter that helps to improve and maintain code quality by checking that the code complies with a customizable subset of its lint rules. Some of the Ruff rules astropy has chosen not to apply at all, but many are currently being ignored simply because no-one has fixed the violations in the astropy code base.

Describe the desired outcome

.ruff.toml in astropy is meant to be a temporary file. All rules listed in that file should eventually be either addressed by editing the astropy source code or permanently moved to pyproject.toml (see #14736). At the moment many of the ignored rules are violated in only a handful of places in the code base, so they can be addressed without too much effort.

Instructions

Selecting a rule to address

To see all the temporarily ignored Ruff rule violations in astropy run

ruff check --config=pyproject.toml --statistics .

This will produce a list of rules that astropy currently does not comply with ordered by the number of violations. It is possible to limit the number of violations Ruff reports to a single sub-package. For example, to see the number of violations in astropy.time run

ruff check --config=pyproject.toml --statistics astropy/time

Use ruff rule to find out more about a rule. For example,

$ ruff rule ISC001
# single-line-implicit-string-concatenation (ISC001)

Derived from the **flake8-implicit-str-concat** linter.

## What it does
Checks for implicitly concatenated strings on a single line.

## Why is this bad?
While it is valid Python syntax to concatenate multiple string or byte
literals implicitly (via whitespace delimiters), it is unnecessary and
negatively affects code readability.

In some cases, the implicit concatenation may also be unintentional, as
autoformatters are capable of introducing single-line implicit
concatenations when collapsing long lines.

## Example
z = "The quick " "brown fox."

Use instead:
z = "The quick brown fox."

If a rule is not in the ignore list in .ruff.toml and is instead in the ignore list in pyproject.toml then it should not be addressed, but including --config=pyproject.toml in the ruff check commands should prevent any such rules from appearing in the output.

Enforcing the rule

Different astropy sub-packages have different maintainers, so to simplify the review process it is preferable to address the rules one sub-package per pull request.

Editing Ruff configuration

Ruff must be told to enforce the rule you choose to address. In .ruff.toml there is a global ignore list and a separate ignore list for each sub-package.

• If the listed in its ignore list then remove it from there.
• If the rule is in the global ignore list then remove it from there and add it to the ignore lists of sub-packages that are violating that rule, but not to the list of the sub-package you will be updating.

Fixing rule violations

Ruff can fix some of the violations automatically, e.g.

ruff check --config=pyproject.toml --select=C417 --fix .

It is highly recommended that you set up pre-commit. If you do then you can fix both any automatically fixable Ruff rule violations and also any code style violations that automatic edits by Ruff might cause by invoking Ruff through the pre-commit framework with

pre-commit run ruff --all

For the rule violations that cannot be automatically fixed their location in the code base can be revealed, e.g.

ruff check --config=pyproject.toml --select=PT015 --output-format=full .

or

ruff check --config=pyproject.toml --select=PTH118 --output-format=full astropy/table

Note that Ruff is in active development and some of the rules might have become automatically fixable in a recent version.

Suggestions for first-time contributors

The following list does not claim to be complete and might be expanded in the future.

• B007
• C408 -- Address Ruff Rule C408 in astropy/io Sub-package #14841, fix: ruff c408 in coordinates submodule #15662, fix: ruff c408 in astropy/units #15664
• EXE002
• PLE0101
• PT022
• RET504
• RET505
• RET506

[pllim]

I thought there were disagreements at Astropy Coordination Meeting 2023 whether this qualifies as "good first issue" or not, because someone unfamiliar with the code won't know if the "corrections" actually introduce bugs?

UPDATE: I will remove the labels for now.

[eerovaher]

Addressing some of the rules can be very complicated and requires familiarity with the package, but many rules are uncontroversial and very suitable for first time contributors. Eventually the simple rules will be addressed and the Package-novice and good first issue labels should be removed, but we are not close to that at the moment.

[pllim]

If you want the "novice" and "good first issue" labels back, please list in your original post clearly which ones are the uncontroversial rules. Thanks!

[Twinotters]

Hi! Hello. I'd love to participate and take this on as a first issue, unless someone is already working on it. The only issue is that I might need some guidance! Thank you in advance.

[nstarman]

Hi @Twinotters, we're always very happy for contributors and you can definitely take on ruff improvements as a first issue. @eerovaher has been kind enough to assemble a list of suggestions for first-time contributors, so if you see something you're interested in, we'd love a PR!

[arthurxvtv]

Hello! I have interest on working on the suggested issues. That being said, i want to know more about how do i approach them. Do i work on them together in the same PR or do i make separately PRs for each issue that i take on? If the latter, do i have to create the respective separate issues here on GitHub before making the PRs?

[nstarman]

Hi @arthurxvtv!

I have interest on working on the suggested issues.

🎉

separately PRs for each issue that i take on?

That's usually preferred. Keep the PRs atomic.

do i have to create the respective separate issues here on GitHub before making the PRs?

I think if you hover over the TaskList that @eerovaher made you can easily convert an item to an Issue. That being said, the TaskList is fine if you don't want to open an issue before the PR.

[nstarman]

@Twinotters @arthurxvtv, just collecting data. As potential contributors, do you think Pinning this issue to the top of the Issues page is useful for discovery of good first issues?

[arthurxvtv]

@pllim I think very it's useful, it can be hard sometimes to discover these issues and the pin could help a lot.

[Mubin17]

Rules	# of Violations
C400	3
C406	7
C408	219
C413	2
C416	39
C414	2
C417	1
ISC001	18
ISC003	17
PIE790	41
PIE804	1
PIE810	1
PLW0120	11
RET504	116
RET505	545
RET506	138

@arthurxvtv I have created a table that compares different rules with the number of violations in the Astropy code base. Based on these numbers you can tackle all issues with less than 20 violations (fetch the rules with less than 20 violations from the above here) in a single PR. I hope this helps. I am planning to fix the rule - C408 sub-package by sub-package.

[pllim]

Re: Issue pin -- I usually only do it if a known issue is critical (e.g., something is horribly broken). If we start pinning issues that we think are good issues, it will be crowded to the point that pinned issues are useless. I suggest you find another way. Thank you for your understanding!

[pllim]

Whoever interested to work on part of this issue, there is no need to open a new issue for each of the small parts. Just open PR and mention this issue. Thanks!

[dhomeier]

astropy has chosen to apply all Ruff rules unless they are explicitly ignored

Is that in summary the outcome of the coordination meeting breakout? It would be good to have this phrased as some form of official astropy policy, as the process is possibly not entirely clear to people who have not been in the breakout.
Also if this indeed means adopting a fluid set of rules with every new ruff release, to have some more info on what governance body is deciding at ruff what rules are introduced or added.

[pllim]

@eerovaher , can we check off C408 yet? Thanks for tracking this!

[nstarman]

@pllim C408 is only addressed in a few subpackages. In most cases it's in the per-package ignore. We should probably keep the reference open, just change the link in the checklist.

[eerovaher]

@pllim asked

can we check off C408 yet?

$ ruff check --select C408 --config pyproject.toml --statistics .
123	C408	[*] Unnecessary `dict` call (rewrite as a literal)

So the answer is no.

[mhvk]

Seeing C408 done in units in #15664, I think we should exclude this rule from being generally enforced: while it makes total sense to replace empty tuple() with (), etc., I like the ability to define kwargs in a way that looks just like what one does when calling a function.

p.s. For coordinates in #15662, most changes were definitely fine, though also there the last two I would have preferred not to have made.

[dhomeier]

Fully agree; I saw a lot of dict(...) declarations removed in io.ascii that I felt had been used by design, and for good reasons.

[nstarman]

So there are 2 good reasons why using the built-in dict constructor {} is better than the dict callable:

1. Speed. It's like 2x faster to use the dict constructor. Applying Ruff C408 is an easy speedup wherever it's fixed.

3. Safety. Python does not protect dict as a symbol so it can be re-assigned. Because of how Python scopes namespaces the following thing can happpen:

In sum, {} is 100% equivalent but 2x faster and safer.

[mhvk]

Yes, in general definitely good to use the direct definition, but the speed enhancement is irrelevant in all the cases in units -- and readability counts!

[dhomeier]

Yes, exactly my point – dict(a=1, b=2) immediately springs to the eye at the first glance on a code section, and there is no potential of confusing it with {'a', 'b'} (though admittedly set is not very widely used in astropy or any code I am familiar with).
And reassigning dict probably has plenty of potential for disaster anyway, even with the callables removed here.

[eerovaher]

It seems to me that most of the instances where you would like to not enforce C408 are in tests. We could then consider ignoring the rule in test files and adding noqa instructions to few specific lines outside tests. If you think this is something we should discuss then I suggest you open a separate issue. Personally I wouldn't mind enforcing C408 in all of astropy.

[dhomeier]

I don't think it is simply an issue of tests vs. functional code – there are instances where readability is sacrificed for sometimes insignificant performance gains (sometimes not so much, for sure) in the latter as well. Though those cases could perhaps be covered with noqa easily enough.

[RahatAhmed171]

Hi, I would like to contribute to fix the remaining issues. Is C408 already assigned to someone? If not, how do I get started? Thanks.

[dhomeier]

I've updated the task list to link the PRs already dealing with C408 (if I have not overlooked any that are not labelled accordingly); except for the units one they have already been merged, so if you check the other subpackages for violations you should be able to find the ones you can still work on.