Add config for ruff (#36503, unbundled)#37452
Conversation
|
@fchapoton Any comments on the specific configuration settings? [Edit: More specific settings in #37453. If there's interest to discuss, let's do that over there.] |
tobiasdiez
added
s: needs work
disputed
|
I've moved |
|
I don't find the other claims there, that somehow |
|
The cherry-picked opinion in #36503 (comment) that pyproject.toml is OK for non–Python project directories does not outweigh my concern (#36503 (comment)) that |
|
And finally, the very point of this PR opened with the intent of reducing the gratuitous friction that halted #36503 is NOT to establish If/when a meaningful |
|
Documentation preview for this PR (built with commit 9914a6c; changes) is ready! 🎉 |
| # https://docs.astral.sh/ruff/configuration/#config-file-discovery | ||
|
|
||
| # Assume Python 3.9 | ||
| target-version = "py39" |
There was a problem hiding this comment.
is this the minimal supported version? This isn't what ships with Sage 10.3, right?
There was a problem hiding this comment.
There was a problem hiding this comment.
Thank you for the context :)
| // List of extensions which should be recommended for developers when a workspace is opened for the first time. | ||
| // See https://go.microsoft.com/fwlink/?LinkId=827846 to learn about workspace recommendations. |
There was a problem hiding this comment.
Comments in JSON is generally not a good idea, right? Maybe this is simply vscode boilerplate though
There was a problem hiding this comment.
This is Microsoft's JSONC
|
This seems reasonable to me as a PR but I'm not sure I should formally approve this as I am not knowledgeable about this part of the sage development. |
|
It seems that @GiacomoPope and I are in agreement. |
|
Not sure I understand the build & test fail though... |
|
The test failure is unrelated -- that's tracked in #37536 |
|
I'm posting to record a vote of -1 on behalf of Tobias Diez. |
|
Did Tobias mention the root cause of the minus one? Are the current ruff settings insufficient for Sage, or does he believe ruff shouldn't be used at all? I would love ruff to become the default linter if only for saving global CPU time with all the CI testing. |
|
@GiacomoPope See ticket description. |
|
I vaguely followed the linked PR but your comment was that extracting out the use of ruff was uncontroversial, so I don't really understand. This seems to make ruff functional for SageMath in a way which doesnt cause an issue -- my question was then why -1 unless there's something wrong with this was of including ruff? |
Adding a ruff configuration is uncontroversial. #36503 demands that it be added in pyproject.toml. My interpretation of the -1 here is that Tobias continues his grandstanding about this. |
|
Regardless of the material of this PR, I am -1 because this part of the PR description disrupts the conventional workflow of sage development. I have the same opinion to all such PRs. |
|
@kwankyu How would you suggest to resolve this? |
|
I've changed it to: "Author: @mkoeppe, based on @tobiasdiez's config in #36503." I do wish to give authorship credit (it is of course also reflected on the commit). |
Yes. Then I retract my vote. That is, no vote. |
|
Based on @kwankyu's suggestion and discussion by the new Code of Conduct Committee, I'm setting this ticket to Needs Review so that all the disputed tickets start with a clean slate prior to being update based on voting. Starting at midnight US/Pacific time tonight anyone involved should feel free to set the status to positive review or needs review in accordance with the new policy. |
|
I think this looks like a sensible way to add ruff support to Sage. I vote +1. |
|
+1 |
1 similar comment
|
👍 |
|
I'm counting:
So I am setting to "positive review". |
sagemathgh-37453: tox.ini: Add environments `ruff`, `ruff-minimal`; GH Actions: run `ruff-minimal` <!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes sagemath#1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> `./sage -tox` and the GH Actions "Lint" workflow now additionally run `ruff-minimal`. The "Lint" workflow outputs GitHub annotations for view in the "Files changed" tab, as pioneered in sagemath#36512 (see screenshot there). Demo: https://github.com/sagemath/sage/pull/37456/files We use the built-in capability of ruff to output via `RUFF_OUTPUT_FORMAT=github` (no problem matcher is needed; see https://github.com/ChartBoost/ruff- action/issues/7#issuecomment-1887780308). (This has been adopted in the revised sagemath#36512.) sagemath#36512 (comment) is marked "disputed" because it builds upon the sagemath#36503, which bundles a controversial design choice, as explained in sagemath#37452. In further contrast to sagemath#36512, we do not remove the pycodestyle-minimal run from the "Lint" workflow. This can be done in a follow-up, once we have gained the necessary experience with the new linter (see previous info request in sagemath#36512 (comment)). Hence I am marking sagemath#36512 not as a "duplicate" but as "pending"; and "disputed" because of its dependency on the "disputed" sagemath#36503. @roed314 @vbraun And in further contrast to sagemath#36512, the minimal ruff configuration used by the CI can be used locally with `./sage -tox -e ruff-minimal` and also runs as part of the default tests in `./sage -tox`. Authors: @mkoeppe, @tobiasdiez (credit for the first version of the minimal ruff configuration taken from sagemath#36512, now regenerated) <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> - Depends on sagemath#37452 <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#37453 Reported by: Matthias Köppe Reviewer(s): Frédéric Chapoton, Kwankyu Lee, Matthias Köppe
sagemathgh-37453: tox.ini: Add environments `ruff`, `ruff-minimal`; GH Actions: run `ruff-minimal` <!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes sagemath#1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> `./sage -tox` and the GH Actions "Lint" workflow now additionally run `ruff-minimal`. The "Lint" workflow outputs GitHub annotations for view in the "Files changed" tab, as pioneered in sagemath#36512 (see screenshot there). Demo: https://github.com/sagemath/sage/pull/37456/files We use the built-in capability of ruff to output via `RUFF_OUTPUT_FORMAT=github` (no problem matcher is needed; see https://github.com/ChartBoost/ruff- action/issues/7#issuecomment-1887780308). (This has been adopted in the revised sagemath#36512.) sagemath#36512 (comment) is marked "disputed" because it builds upon the sagemath#36503, which bundles a controversial design choice, as explained in sagemath#37452. In further contrast to sagemath#36512, we do not remove the pycodestyle-minimal run from the "Lint" workflow. This can be done in a follow-up, once we have gained the necessary experience with the new linter (see previous info request in sagemath#36512 (comment)). Hence I am marking sagemath#36512 not as a "duplicate" but as "pending"; and "disputed" because of its dependency on the "disputed" sagemath#36503. @roed314 @vbraun And in further contrast to sagemath#36512, the minimal ruff configuration used by the CI can be used locally with `./sage -tox -e ruff-minimal` and also runs as part of the default tests in `./sage -tox`. Authors: @mkoeppe, @tobiasdiez (credit for the first version of the minimal ruff configuration taken from sagemath#36512, now regenerated) <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> - Depends on sagemath#37452 <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#37453 Reported by: Matthias Köppe Reviewer(s): Frédéric Chapoton, Kwankyu Lee, Matthias Köppe
10 participants
Author: @mkoeppe, based on @tobiasdiez's config in #36503.
Adding a configuration for the ruff linter as proposed in #36503 is timely and uncontroversial.
However, #36503 bundled this gratuitously with the controversial design of creating a
pyproject.tomlfile in SAGE_ROOT.pyproject.toml-- by definition in PEP 518, PEP 621 -- marks a directory as a Python project, i.e., the source directory of a Python distribution package (#36503 (comment)).Generalizing the use of
pyproject.tomlto "non-package projects" is still subject to discussion in the Python community in PEP 735 (Nov 2023).The scope of PRs should be chosen to minimize friction, not to maximize friction.
#36726 (comment)
Here we remove the artificial friction from the gratuitous bundling, by configuring ruff in
ruff.tomlinstead. Configuration in ruff.toml and pyproject.toml has equal validity / standing per ruff documentation. And this is the location of most of our other linter configuration files.Reference on previous common denominator PRs: #36666 (comment), #36666 (comment), #36572 (comment), #36960 (comment), #36960 (comment), ...
📝 Checklist
⌛ Dependencies