Add config for ruff #36503
Add config for ruff #36503
Conversation
| @@ -0,0 +1,13 @@ | |||
| [tool.ruff] | |||
There was a problem hiding this comment.
Please, no pyproject.toml in SAGE_ROOT.
SAGE_ROOT is not a Python package.
There was a problem hiding this comment.
It applies to all python code (e.g. also the one in pkgs), so should be in the root. It's also quite common that the pyproject config is in the root, but the actual code in a subfolder. eg https://github.com/scipy/scipy/blob/main/pyproject.toml or https://github.com/pandas-dev/pandas/blob/main/pyproject.toml
There was a problem hiding this comment.
It applies to all python code (e.g. also the one in pkgs), so should be in the root.
No, that's not how the Sage repository is structured. Again, SAGE_ROOT is not the source directory of a Python package (distribution)... which is why there is no setup.py, no pyproject.toml, no setup.cfg, etc.
There was a problem hiding this comment.
How is that different from scipy?
There was a problem hiding this comment.
The source directory of the Python distribution package scipy (per PEP 518, PEP 621, the location of the pyproject.toml file) is the root of its repository https://github.com/scipy/scipy/tree/main
The source directory of the Python distribution package sagemath-standard is not the root of the repository https://github.com/sagemath/sage (= SAGE_ROOT), but rather src/. As I know that you know, this is where you find src/pyproject.toml, src/setup.py, etc.
There was a problem hiding this comment.
Not everything that has a pyproject file is a "distribution package".
That's the very idea of pyproject.toml. It marks the top of a Python distribution package (= project). That's what "py" and "project" refer to in the name of this file.
There was a problem hiding this comment.
Some special distribution packages (currently only pkgs/sage-sws2rst, pkgs/sage-conf*) do have their own source files, so tool configuration could go into pkgs/sage-sws2rst/pyproject.toml etc.
That doesn't scale.
It does not have to scale. As I just explained to you, there is a fixed short list of packages.
Copying the config 4 times is already too much for my taste. Thanks for the suggestion, but I'll not implement it.
There was a problem hiding this comment.
So do ruff.toml, as I explained.
There was a problem hiding this comment.
So do ruff.toml, as I explained.
I've tried that but couldn't make it work reliable. For some files it just doesn't seem to work (although, I agree, that the docs claim that ruff.toml and pyproject.toml are equivalent). As a starter, you need to put the config in the root so that it applies to all python files in all directories. Moreover, it sometimes just didn't apply the config when reformatting a file - not sure if it's a problem with ruff or vscode, or something else. My guess is that the somewhat strange directory layout with a pyproject.toml in src is resulting in hick ups.
Since the config in the root pyproject.toml worked exellently for me, I stopped investigating. The added pyproject.toml in the root doesn't cause any other issues, is helpful in other contexts as well (eg #37446) and pip install . will work correctly with #36524.
Sorry, should have let you know before you invested the time to prepare #37452.
There was a problem hiding this comment.
Not everything that has a pyproject file is a "distribution package".
That's the very idea of pyproject.toml. It marks the top of a Python distribution package (= project). That's what "py" and "project" refer to in the name of this file.
That's a very narrow viewpoint. Multiple tools support non-distribution/non-package scenarios via pyproject.toml, see eg https://python-poetry.org/docs/basic-usage#operating-modes. In https://discuss.python.org/t/projects-that-arent-meant-to-generate-a-wheel-and-pyproject-toml/29684/5 a Cpython core developer addressed the concern that pyproject.toml indicates that code which will be installed as a distribution as follows:
[...] storing configuration for black/ruff etc is not a reasonable indicator that code contained in that directory is intended to be installed as a distribution. The project section, yes, is currently only usable with distribution; but it’s certainly not an indicator without that. [...] The concern was (as I understand it) that pyproject.toml indicates that code which will be installed as a distribution. That isn’t the case and it’s not a packaging-exclusive marker file today; and it hasn’t been since its introduction.
In the same thread, another Cpython core developer answers the question "if a project (in the broadest possible sense of the term) is not intended to be built into a wheel, is pyproject.toml an appropriate place to store project-level configuration?" with
I think so. With the level of adoption of [tool] I think this has already happened.
According to this viewpoint, since I don't add a project section, the pyproject.toml in the root does not function as a indicator for a distribution package and it's perfectly fine to add tool configs there.
|
Next step = create a tox environment. |
|
I think I was quite clear about what the work item is. |
|
I've already thanked you for that suggestion, but I don't think its a good idea. |
|
No, Tobias, you haven't. You were referring to the solution of putting tool configuration into distribution-package-specific pyproject.toml files in |
|
this link does not point to anywhere remotely related to this PR. |
|
No idea what happened there, but here is the correct link: #36503 (comment) |
vbraun
force-pushed
the
develop
branch
2 times, most recently
from
883e05f
to
e349b00
Compare
tobiasdiez
added
the
disputed
|
@vbraun would be nice to get this in. Since it's only a bunch of config files updates, it should be safe to be marked as "blocker". |
|
Creating a top-level file pyproject.toml just to support an editor/linter/etc is a really bad idea. |
|
Documentation preview for this PR (built with commit 259f429; changes) is ready! 🎉 |
|
The pyproject.toml file definitely does not belong in SAGE_ROOT. But it is true, and somewhat confusing, that the directory structure used in sage is non-standard. I think it would be more standard with the following changes:
Then the setup.py and setup.cfg would be in the project root directory, and the sage source code would be in the src/sage subdirectory of the project root. I think that would be closer to what is suggested in the packaging guide Because pyproject.toml definitely does not belong in SAGE_ROOT I vote -1 on this PR. |
|
👎 nothing terribly wrong with this PR I think but #37452 seems like a cleaner solution to me. |
<!-- ^^^^^ 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 --> Author: @mkoeppe, based on @tobiasdiez's config in sagemath#36503. Adding a configuration for the ruff linter as proposed in sagemath#36503 is timely and uncontroversial. However, sagemath#36503 bundled this gratuitously with the controversial design of creating a `pyproject.toml` file 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 (sagemath#36503 (comment)). Generalizing the use of `pyproject.toml` to "[non-package projects](https://peps.python.org/pep-0735/#motivation)" is still subject to discussion in the Python community in [PEP 735](https://peps.python.org/pep-0735/) (Nov 2023). **The scope of PRs should be chosen to minimize friction, not to maximize friction.** sagemath#36726 (comment) Here we remove the artificial friction from the gratuitous bundling, by configuring ruff in `ruff.toml` instead. Configuration in ruff.toml and pyproject.toml has equal validity / standing per [ruff documentation](https://docs.astral.sh/ruff/configuration/#config-file- discovery). And this is the location of most of our other linter configuration files. Reference on previous common denominator PRs: sagemath#36666 (comment), sagemath#36666 (comment), sagemath#36572 (comment), sagemath#36960 (comment), sagemath#36960 (comment), ... <!-- 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: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#37452 Reported by: Matthias Köppe Reviewer(s): Giacomo Pope, 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
…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
Vscode removed the (built-in) support for pycodestyle and now recommends to use ruff.. For this reason, we add ruff config. The config corresponds to an "ideal version" (instead of a minimal version that is in the linter-ci), so that devs don't introduce unnecessary new issues that would need fixing later.
There are probably a few rules that we can disable as they don't apply / cause to many false negatives. I would say we start with this maximal config and then disable these offending rules later as we get more experience. A notable exception is the line length rule, which I've directly disabled as there is no point in enforcing it currently.
Next step: use it in ci, this is now #36512
📝 Checklist
⌛ Dependencies