-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ensure compliance with more Ruff rules #14818
Comments
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. |
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 |
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! |
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. |
Hi @Twinotters, we're always very happy for contributors and you can definitely take on |
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? |
Hi @arthurxvtv!
🎉
That's usually preferred. Keep the PRs atomic.
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. |
@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? |
@pllim I think very it's useful, it can be hard sometimes to discover these issues and the pin could help a lot. |
@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. |
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! |
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! |
Currently the selected files violate Ruff rule C416: - astropy/config/tests/test_configs.py - astropy/io/ascii/core.py - astropy/io/ascii/ipac.py - astropy/io/fits/tests/test_core.py - astropy/io/fits/tests/test_hdulist.py - astropy/modeling/tests/test_bounding_box.py - astropy/modeling/tests/test_polynomial.py - astropy/nddata/bitmask.py - astropy/nddata/tests/test_nddata.py - astropy/table/bst.py - astropy/table/column.py - astropy/table/table.py - astropy/table/table_helpers.py - astropy/table/tests/test_array.py - astropy/table/tests/test_index.py - astropy/table/tests/test_row.py - astropy/table/tests/test_table.py - astropy/time/formats.py - astropy/units/tests/test_physical.py - astropy/units/tests/test_quantity_array_methods.py Rule C416 checks for unnecessary dict, list, and set comprehension.
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. |
@eerovaher , can we check off C408 yet? Thanks for tracking this! |
@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. |
@pllim asked
$ ruff check --select C408 --config pyproject.toml --statistics .
123 C408 [*] Unnecessary `dict` call (rewrite as a literal) So the answer is no. |
Seeing 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. |
Fully agree; I saw a lot of |
So there are 2 good reasons why using the built-in dict constructor
In sum, |
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! |
Yes, exactly my point – |
It seems to me that most of the instances where you would like to not enforce |
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 |
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. |
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 |
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 theastropy
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 theastropy
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
runruff 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 inastropy.time
runUse
ruff rule
to find out more about a rule. For example,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 theruff 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.
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 thepre-commit
framework withFor 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
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.
The text was updated successfully, but these errors were encountered: