Skip to content

Fix and enable more ruff rules #41166

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

Jump to bottom
Open
wants to merge 18 commits into
base: develop
Choose a base branch
from
Open

Fix and enable more ruff rules #41166

wants to merge 18 commits into from
+141 −105

Conversation

@vincentmacri
Copy link
Member

@vincentmacri vincentmacri commented Nov 12, 2025 •
edited
Loading

Fix and enable RUF013, RUF036, UP004, UP008, UP010, UP015, UP022, UP034, and UP035.

Perhaps the most important of these is RUF013, which fixes implicit Optional in type annotations, as this is prohibited by PEP484. I also sorted the imports in the files that were changed in the process of fixing these rules.

📝 Checklist

  • The title is concise and informative.
  • 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 and checked the documentation preview.

⌛ Dependencies

vincentmacri
vincentmacri commented Nov 12, 2025
View reviewed changes
.github/workflows/lint.yml Outdated
run: |
uv run --frozen --only-group lint -- ruff check --output-format github --ignore E402,E721,E731,E741,E742,E743,F401,F402,F403,F405,F821,F841,I001,PLC0206,PLC0208,PLC1802,PLC2401,PLC3002,PLC0415,PLE0302,PLR0124,PLR0402,PLR0911,PLR0912,PLR0913,PLR0915,PLR1704,PLR1711,PLR1714,PLR1716,PLR1733,PLR1736,PLR2004,PLR5501,PLW0120,PLW0211,PLW0602,PLW0603,PLW0642,PLW1508,PLW1510,PLW1641,PLW2901,PLW3301
uv run --frozen --only-group lint -- ruff check --output-format github --preview --select E111,E115,E21,E221,E222,E225,E227,E228,E25,E271,E272,E275,E302,E303,E305,E306,E401,E502,E701,E702,E703,E71,W291,W293,W391,W605,TC,UP006 src/sage/
uv run --frozen --only-group lint -- ruff check --output-format github --ignore E402,E721,E731,E741,E742,E743,F401,F402,F403,F405,F821,F841,I001,PLC0206,PLC0208,PLC1802,PLC2401,PLC3002,PLC0415,PLE0302,PLR0124,PLR0402,PLR0911,PLR0912,PLR0913,PLR0915,PLR1704,PLR1711,PLR1714,PLR1716,PLR1733,PLR1736,PLR2004,PLR5501,PLW0120,PLW0211,PLW0602,PLW0603,PLW0642,PLW1508,PLW1510,PLW1641,PLW2901,PLW3301,UP
Copy link
Member Author

@vincentmacri vincentmacri Nov 12, 2025

Choose a reason for hiding this comment

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

Disabling UP here because this command checks build/sage_bootstrap. I don't know what the purpose of that folder is, but the README in it says it needs to support Python 2.6 (surely that can't be right...) and when I tried running the ruff fixer on that folder things broke.

@tobiasdiez Do you know what that folder is for or why this workflow runs ruff twice?

Copy link
Contributor

@tobiasdiez tobiasdiez Nov 13, 2025

Choose a reason for hiding this comment

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

It's the Python interface for the sage-packages and it is called by some of the shell scripts in bin, which themselves are invoked during bootstrap. Since sage-the-distro officially supports bootstrapping itself on very old Python's, the 2.6 requirement sounds plausible to me.
I think we should just hide that folder from ruff.

For the two commands, I think the difference is the --preview flag, or not?

Copy link
Member Author

@vincentmacri vincentmacri Nov 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think we should just hide that folder from ruff.

Done

For the two commands, I think the difference is the --preview flag, or not?

There is more than that. The first one has --ignore rules, the second one has --select rules. The second one runs on src/sage/ and the first runs on the entire repo (I've now changed it to run on src/ to address your other comment).

@vincentmacri vincentmacri added t: refactoring github_actions Pull requests that update GitHub Actions code labels Nov 12, 2025
@github-actions
Copy link

github-actions bot commented Nov 12, 2025 •
edited
Loading

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

@vincentmacri
Copy link
Member Author

vincentmacri commented Nov 13, 2025 •
edited
Loading

Running UP on src/ caused a lot of lint failures in conf.py files in src/doc/en/reference/. I think ruff can autofix all of them, but that will mean a lot more changed files in this PR.

@tobiasdiez Do you want me to disable UP for that check (it's running on src/src anyway), or do you want to review what I have now, then if it's good I can have ruff fix the failures and then you can look at those changes as a separate commit in this PR (just to make it easier for you to review).

@vincentmacri
Copy link
Member Author

vincentmacri commented Nov 13, 2025

I think the test failures for src/sage_docbuild/builders.py are real but unrelated to this PR. It occurs in one of my other development branches as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tobiasdiez tobiasdiez Awaiting requested review from tobiasdiez

Assignees

No one assigned

Labels

github_actions Pull requests that update GitHub Actions code s: needs review t: refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vincentmacri @tobiasdiez