Repository navigation
Ensure compliance with more Ruff rules #14818
Description
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/timeUse 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 rule is listed in the ignore list of a sub-package you will be updating 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 --all-files ruffIf Ruff cannot apply automatic fixes then the code needs to be edited manually, but Ruff can still be used to find the violations.
Suggestions for first-time contributors
The following list does not claim to be complete and might be expanded in the future.
- B007 fix ruff rule B007 violation in astropy/modeling #17860, fix ruff rule B007 violation in astropy/wcs #17866, fix ruff rule B007 violation in astropy/coordinates #17878, fix ruff rule B007 violation in astropy/io #17880, fix ruff rule B007 violation in astropy/samp #17881
- 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, fix: ruff rule c408 in astropy/cosmology #17791, fix ruff rule c408 violations in all remaining subpackages #17798
- EXE002
- PLE0101 Fix Ruff rule PLE0101 violations #17814
- PT022 Fix Ruff rule PT022 violations #17479
- RET504 Fix Ruff rule RET504 violations in astropy/visualization #17818
Activity
pllim commentedon May 12, 2023
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 commentedon May 12, 2023
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-noviceandgood first issuelabels should be removed, but we are not close to that at the moment.pllim commentedon May 12, 2023
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 commentedon May 12, 2023
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 commentedon May 15, 2023
Hi @Twinotters, we're always very happy for contributors and you can definitely take on
ruffimprovements 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!60 remaining items
eerovaher commentedon Feb 19, 2025
The rules listed in the opening post are all uncontroversial. The disagreements around C408 (unnecessary-collection-call) were resolved by turning on the allow-dict-calls-with-keyword-arguments setting.
VarunNikam22 commentedon Feb 20, 2025
Is it better to fix these violations sub-package by sub-package or rule-by-rule? I started taking up
astropy/constantsbut then noticed that there are also some global ignored rules which are also violated inconstantsand need to be addressed too. Should I address them separately or in the same PR?Please advise.
neutrinoceros commentedon Feb 20, 2025
It's best to issue fixes per sub-package, as your PRs might otherwise get blocked by just one missing reviewer. I merged your first PR quickly because I really didn't see how it could be controversial but generally it's best to target just one subpackage at a time when possible. Thanks !
VarunNikam22 commentedon Feb 20, 2025
How do you think I should I proceed with the global ignore rules then, since they would also affect other subpackages I'm not editing?
Or shall I fix the per-subpackage list first and deal with the global rules later after all?
neutrinoceros commentedon Feb 20, 2025
It depends on how many subpackages are affected, and the total size of the patch: I'd say if there are more than a couple lines affected per subpackage, or if more than, say, 3 or 4 subpackages need updates, then the patch should be issued piecemeal, starting with a PR that replaces the global setting with as many local ones as necessary.
VarunNikam22 commentedon Feb 20, 2025
Quick question: Are all the rules mentioned in .ruff.toml to be enforced across the package? How do we decide on ambiguous cases, if any?
For example,
k_Bfor Boltzmann constant and instead suggests something likek_b; but I guess that would change the way the constants are imported by users and need to be documented tooPlease advise.
neutrinoceros commentedon Feb 20, 2025
re: E402
I don't think we want to comply with this rule for
__init__.pyfiles. I'd argue it should be moved to permanent settings.re: N816
This is even clearer: API stability is (by far) more important than lint compliance, so I think we should permanently ignore this rule, at least for public APIs. All existing violations in
astropy.constants,astropy.coordinatesandastropy.unitsshould be ignored, and I think it makes sense to disable the rule completely for these subpackages. Other violations are either on private APIs or tests, so it should be fine to fix them.VarunNikam22 commentedon Feb 20, 2025
@neutrinoceros Any reason RET505 was left in
.ruff.tomleven after making it permanent in commit cb0685f ?eerovaher commentedon Feb 20, 2025
The rules in
.ruff.tomlshould be considered to be untriaged, except the small number of rules listed in the opening post that we are sure we want to enforce in all sub-packages.There is no simple solution to this. If there were then we would have done it already. If you are unsure about a rule then I would recommend opening a pull request for the sub-package that has the fewest violations of that rule.
As long as our documentation recommends defining configuration items in
__init__.pyfiles, ignoring E402 (module-import-not-at-top-of-file) in them should be a part of our permanent configuration (inpyproject.toml).Sometimes Python naming conventions are in conflict with domain-specific naming conventions and often the latter are more important. N816 (mixed-case-variable-in-global-scope) should be ignored in many places, but it is not immediately obvious to me what would be the best way to implement those ignores.
kangqiwang commentedon Feb 25, 2025
update EXE002 shebang missing base on astropy#14818
eteq commentedon Jun 24, 2025
As discussed at Astropy 2025 coordination meeting, we think the low-hanging fruit are done, and we no longer want to encourage this as good first issue because it's diminishing returns.