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

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Feb 24, 2024

./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 #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 ChartBoost/ruff-action#7 (comment)). (This has been adopted in the revised #36512.)

#36512 (comment) is marked "disputed" because it builds upon the #36503, which bundles a controversial design choice, as explained in #37452.

In further contrast to #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 #36512 (comment)). Hence I am marking #36512 not as a "duplicate" but as "pending"; and "disputed" because of its dependency on the "disputed" #36503. @roed314 @vbraun

And in further contrast to #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 #36512, now regenerated)

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@mkoeppe mkoeppe self-assigned this Feb 24, 2024
@mkoeppe mkoeppe changed the title tox.ini: Add environments ruff, ruff-minimal tox.ini: Add environments ruff, ruff-minimal; GH Actions: run ruff-minimal Feb 24, 2024
@mkoeppe mkoeppe requested a review from culler March 16, 2024 17:47
@culler
Copy link
Contributor

culler commented Mar 16, 2024

I am not experienced with tox, but the form of the setup looks good to me.

@mkoeppe mkoeppe force-pushed the tox-ruff branch 2 times, most recently from 617b9f6 to eacdb0a Compare April 1, 2024 00:46
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 1, 2024

Rebased.

Copy link

github-actions bot commented Apr 1, 2024

Documentation preview for this PR (built with commit c97451d; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

# 1 PLC0208 [*] Use a sequence type instead of a `set` when iterating over values
#
commands = ruff --ignore I001,PLR2004,F401,E741,F821,PLR0912,PLR0913,E402,PLR0915,PLW2901,PLR5501,PLR0911,E731,F405,PLR1714,F403,PLR0402,PLW0603,E713,F841,PLW0602,E714,PLR1711,PLR1701,PLW3301,E721,PLW1510,F811,PLW0120,PLC0414,E743,F541,PLE0101,PLR0124,PLW0127,PLW1508,PLC3002,E742,PLE0302,F402,PLC0208,PLW0129 {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

# 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

@kwankyu
Copy link
Collaborator

kwankyu commented Apr 30, 2024

I get

$ ./sage -tox -e ruff-minimal
ruff-minimal: commands[0] /Users/kwankyu/GitHub/sage-temp/src> ruff --ignore I001,PLR2004,F401,E741,F821,PLR0912,PLR0913,E402,PLR0915,PLW2901,PLR5501,PLR0911,E731,F405,PLR1714,F403,PLR0402,PLW0603,E713,F841,PLW0602,E714,PLR1711,PLR1701,PLW3301,E721,PLW1510,F811,PLW0120,PLC0414,E743,F541,PLE0101,PLR0124,PLW0127,PLW1508,PLC3002,E742,PLE0302,F402,PLC0208,PLW0129 /Users/kwankyu/GitHub/sage-temp/src/sage/
warning: `ruff <path>` is deprecated. Use `ruff check <path>` instead.
All checks passed!
  ruff-minimal: OK (0.29=setup[0.08]+cmd[0.21] seconds)
  congratulations :) (0.40 seconds)

Note the warning.

Copy link
Contributor

@fchapoton fchapoton left a comment

Choose a reason for hiding this comment

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

ok, let's move on

vbraun pushed a commit to vbraun/sage that referenced this pull request May 18, 2024
…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
@vbraun vbraun merged commit fec763c into sagemath:develop May 25, 2024
16 of 17 checks passed
@mkoeppe mkoeppe deleted the tox-ruff branch May 25, 2024 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants