Skip to content
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.

Sign up for GitHub

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

Add config for ruff #36503

Closed
wants to merge 9 commits into from
Closed

Add config for ruff #36503

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6283126
Add config for ruff
tobiasdiez Oct 22, 2023
3a3a5ed
Merge branch 'develop' into ruff-vscode
tobiasdiez Nov 5, 2023
8523cc6
Merge branch 'develop' into ruff-vscode
tobiasdiez Nov 10, 2023
48ba573
Merge branch 'develop' into ruff-vscode
tobiasdiez Nov 17, 2023
1170198
Merge branch 'develop' into ruff-vscode
tobiasdiez Dec 10, 2023
8c1c34d
Merge branch 'develop' into ruff-vscode
tobiasdiez Jan 22, 2024
11d563c
Merge branch 'develop' into ruff-vscode
tobiasdiez Feb 11, 2024
4314ba5
Merge branch 'develop' into ruff-vscode
tobiasdiez Feb 25, 2024
259f429
Put select and ignore in `lint` section to silence deprecation warning
tobiasdiez Mar 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions .devcontainer/devcontainer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,12 @@
"vscode": {
"extensions": [
"guyskk.language-cython",
"ms-python.isort",
"ms-toolsai.jupyter",
"ms-python.vscode-pylance",
"ms-python.pylint",
"ms-python.python",
"lextudio.restructuredtext",
"trond-snekvik.simple-rst"
"trond-snekvik.simple-rst",
"charliermarsh.ruff"
]
}
}
Expand Down
3 changes: 2 additions & 1 deletion .vscode/extensions.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// 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.
"recommendations": [
"ms-python.python"
"ms-python.python",
"charliermarsh.ruff"
],
}
5 changes: 0 additions & 5 deletions .vscode/settings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,6 @@
"--doctest-modules"
],
"python.testing.unittestEnabled": false,
"python.linting.pycodestyleEnabled": true,
"python.linting.enabled": true,
// The following pycodestyle arguments are the same as the pycodestyle-minimal
// tox environnment, see the file SAGE_ROOT/src/tox.ini
"python.linting.pycodestyleArgs": ["--select= E111,E21,E222,E225,E227,E228,E25,E271,E303,E305,E306,E401,E502,E701,E702,E703,E71,E72,W291,W293,W391,W605"],
"cSpell.words": [
"furo",
"Conda",
Expand Down
13 changes: 13 additions & 0 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
[tool.ruff]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, no pyproject.toml in SAGE_ROOT.
SAGE_ROOT is not a Python package.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is that different from scipy?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So do ruff.toml, as I explained.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@tobiasdiez tobiasdiez Feb 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# Assume Python 3.9
target-version = "py39"

select = [
"E", # pycodestyle errors - https://docs.astral.sh/ruff/rules/#error-e
"F", # pyflakes - https://docs.astral.sh/ruff/rules/#pyflakes-f
"I", # isort - https://docs.astral.sh/ruff/rules/#isort-i
"PL", # pylint - https://docs.astral.sh/ruff/rules/#pylint-pl
]
ignore = [
"E501", # Line too long - hard to avoid in doctests, and better handled by black.
]
Loading