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.] |
mkoeppe
force-pushed
the
36503-unbundled
branch
from
0d6c693
to
cbf8411
Compare
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 |
|
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". |
…H 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
…H 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
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