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.

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

tox.ini: Add environments ruff, ruff-minimal; GH Actions: run ruff-minimal #37453

Merged
merged 9 commits into from
May 25, 2024
9 changes: 8 additions & 1 deletion .github/workflows/lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,14 @@ jobs:
id: deps
run: pip install tox

- name: Code style check with pycodestyle
- name: Code style check with ruff-minimal
if: (success() || failure()) && steps.deps.outcome == 'success'
run: tox -e ruff-minimal
env:
# https://github.com/ChartBoost/ruff-action/issues/7#issuecomment-1887780308
RUFF_OUTPUT_FORMAT: github

- name: Code style check with pycodestyle-minimal
if: (success() || failure()) && steps.deps.outcome == 'success'
run: tox -e pycodestyle-minimal

Expand Down
4 changes: 2 additions & 2 deletions ruff.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@
# Assume Python 3.9
target-version = "py39"

select = [
lint.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 = [
lint.ignore = [
"E501", # Line too long - hard to avoid in doctests, and better handled by black.
]
18 changes: 17 additions & 1 deletion src/doc/en/developer/tools.rst
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ available::
--tox [options] <files|dirs> -- general entry point for testing
and linting of the Sage library
-e <envlist> -- run specific test environments; default:
doctest,coverage,startuptime,pycodestyle-minimal,relint,codespell,rst
doctest,coverage,startuptime,pycodestyle-minimal,relint,codespell,rst,ruff-minimal
doctest -- run the Sage doctester
(same as "sage -t")
coverage -- give information about doctest coverage of files
Expand All @@ -60,11 +60,13 @@ available::
relint -- check whether some forbidden patterns appear
codespell -- check for misspelled words in source code
rst -- validate Python docstrings markup as reStructuredText
ruff-minimal -- check against Sage's minimal style conventions
coverage.py -- run the Sage doctester with Coverage.py
coverage.py-html -- run the Sage doctester with Coverage.py, generate HTML report
pyright -- run the static typing checker pyright
pycodestyle -- check against the Python style conventions of PEP8
cython-lint -- check Cython files for code style
ruff -- check against Python style conventions
-p auto -- run test environments in parallel
--help -- show tox help

Expand Down Expand Up @@ -287,6 +289,20 @@ for Python code, written in Rust.
It comes with a large choice of possible checks, and has the capacity
to fix some of the warnings it emits.

Sage defines two configurations for ruff. The command ``./sage -tox -e ruff-minimal`` uses
ruff in a minimal configuration. As of Sage 10.3, the entire Sage library conforms to this
configuration. When preparing a Sage PR, developers should verify that
``./sage -tox -e ruff-minimal`` passes.

The second configuration is used with the command ``./sage -tox -e ruff`` and runs a
more thorough check. When preparing a PR that adds new code,
developers should verify that ``./sage -tox -e ruff`` does not
issue warnings for the added code. This will avoid later cleanup
PRs as the Sage codebase is moving toward full PEP 8 compliance.

On the other hand, it is usually not advisable to mix coding-style
fixes with productive changes on the same PR because this would
makes it harder for reviewers to evaluate the changes.

.. _section-tools-relint:

Expand Down
62 changes: 61 additions & 1 deletion src/tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
## in a virtual environment.
##
[tox]
envlist = doctest, coverage, startuptime, pycodestyle-minimal, relint, codespell, rst
envlist = doctest, coverage, startuptime, pycodestyle-minimal, relint, codespell, rst, ruff-minimal
# When adding environments above, also update the delegations in SAGE_ROOT/tox.ini
skipsdist = true

Expand Down Expand Up @@ -259,6 +259,66 @@ description =
deps = cython-lint
commands = cython-lint --no-pycodestyle {posargs:{toxinidir}/sage/}

[testenv:ruff]
description =
check against Python style conventions
deps = ruff
passenv = RUFF_OUTPUT_FORMAT
commands = ruff check {posargs:{toxinidir}/sage/}

[testenv:ruff-minimal]
description =
check against Sage's minimal style conventions
deps = ruff
# https://github.com/ChartBoost/ruff-action/issues/7#issuecomment-1887780308
passenv = RUFF_OUTPUT_FORMAT
# Output of currently failing, from "./sage -tox -e ruff -- --statistics":
#
# 3579 PLR2004 [ ] Magic value used in comparison, consider replacing `- 0.5` with a constant variable
# 3498 I001 [*] Import block is un-sorted or un-formatted
# 2146 F401 [*] `.PyPolyBoRi.Monomial` imported but unused
# 1964 E741 [ ] Ambiguous variable name: `I`
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does [*] mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

the star usually means auto-fixable

# 1676 F821 [ ] Undefined name `AA`
# 1513 PLR0912 [ ] Too many branches (102 > 12)
# 1159 PLR0913 [ ] Too many arguments in function definition (10 > 5)
# 815 E402 [ ] Module level import not at top of file
# 671 PLR0915 [ ] Too many statements (100 > 50)
# 483 PLW2901 [ ] Outer `for` loop variable `ext` overwritten by inner `for` loop target
# 433 PLR5501 [*] Use `elif` instead of `else` then `if`, to reduce indentation
# 428 PLR0911 [ ] Too many return statements (10 > 6)
# 404 E731 [*] Do not assign a `lambda` expression, use a `def`
# 351 F405 [ ] `ComplexField` may be undefined, or defined from star imports
# 306 PLR1714 [*] Consider merging multiple comparisons. Use a `set` if the elements are hashable.
# 236 F403 [ ] `from .abelian_gps.all import *` used; unable to detect undefined names
# 116 PLR0402 [*] Use `from matplotlib import cm` in lieu of alias
# 111 PLW0603 [ ] Using the global statement to update `AA_0` is discouraged
# 78 F841 [*] Local variable `B` is assigned to but never used
# 64 E713 [*] Test for membership should be `not in`
# 48 PLW0602 [ ] Using global for `D` but no assignment is done
# 33 PLR1711 [*] Useless `return` statement at end of function
# 24 E714 [*] Test for object identity should be `is not`
# 20 PLR1701 [*] Merge `isinstance` calls
# 17 PLW3301 [ ] Nested `max` calls can be flattened
# 16 PLW1510 [*] `subprocess.run` without explicit `check` argument
# 14 E721 [ ] Do not compare types, use `isinstance()`
# 14 PLW0120 [*] `else` clause on loop without a `break` statement; remove the `else` and dedent its contents
# 12 F811 [*] Redefinition of unused `CompleteDiscreteValuationRings` from line 49
# 8 PLC0414 [*] Import alias does not rename original package
# 7 E743 [ ] Ambiguous function name: `I`
# 7 PLE0101 [ ] Explicit return in `__init__`
# 7 PLR0124 [ ] Name compared with itself, consider replacing `a == a`
# 5 PLW0127 [ ] Self-assignment of variable `a`
# 4 F541 [*] f-string without any placeholders
# 4 PLW1508 [ ] Invalid type for environment variable default; expected `str` or `None`
# 3 PLC3002 [ ] Lambda expression called directly. Execute the expression inline instead.
# 2 E742 [ ] Ambiguous class name: `I`
# 2 PLE0302 [ ] The special method `__len__` expects 1 parameter, 3 were given
# 2 PLW0129 [ ] Asserting on a non-empty string literal will always pass
# 1 F402 [ ] Import `factor` from line 259 shadowed by loop variable
# 1 PLC0208 [*] Use a sequence type instead of a `set` when iterating over values
#
commands = ruff check --ignore PLR2004,I001,F401,E741,F821,PLR0912,PLR0913,E402,PLR0915,PLW2901,PLR5501,PLR0911,E731,F405,PLR1714,F403,PLR0402,PLW0603,F841,E713,PLW0602,PLR1711,E714,PLR1701,PLW3301,PLW1510,E721,PLW0120,F811,PLC0414,E743,PLE0101,PLR0124,PLW0127,F541,PLW1508,PLC3002,E742,PLE0302,PLW0129,F402,PLC0208 {posargs:{toxinidir}/sage/}

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about making the list in line 300 is ordered the same with the list above, for easy maintenance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, done in c97451d

[flake8]
rst-roles =
# Sphinx
Expand Down
20 changes: 20 additions & 0 deletions tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -1090,3 +1090,23 @@ passenv = {[sage_src]passenv}
envdir = {[sage_src]envdir}
commands = {[sage_src]commands}
allowlist_externals = {[sage_src]allowlist_externals}

[testenv:ruff]
description =
check against Python style conventions
passenv = {[sage_src]passenv}
# https://github.com/ChartBoost/ruff-action/issues/7#issuecomment-1887780308
RUFF_OUTPUT_FORMAT
envdir = {[sage_src]envdir}
allowlist_externals = {[sage_src]allowlist_externals}
commands = tox -c {toxinidir}/src/tox.ini -e {envname} -- {posargs:src/sage/}

[testenv:ruff-minimal]
description =
check against Sage's minimal style conventions
passenv = {[sage_src]passenv}
# https://github.com/ChartBoost/ruff-action/issues/7#issuecomment-1887780308
RUFF_OUTPUT_FORMAT
envdir = {[sage_src]envdir}
allowlist_externals = {[sage_src]allowlist_externals}
commands = tox -c {toxinidir}/src/tox.ini -e {envname} -- {posargs:src/sage/}
Loading