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

fix ruff PLR1730 checks #38540

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

GiacomoPope
Copy link
Contributor

@GiacomoPope GiacomoPope commented Aug 21, 2024

In many places we use comparisons followed by conditional assignment. PLR1730 identifies this and we can instead use min and max

With #38541 this should fix the three errors from the lint CI (run from develop)

Jack: sage % ruff check --ignore PLR2004,I001,F401,E741,F821,PLR0912,PLR0913,E402,PLR0915,PLW2901,PLR5501,PLR0911,E731,F405,PLR1714,PLR1736,F403,PLR0402,PLW0603,F841,E713,PLW0602,PLR1711,E714,PLR1701,PLR1704,PLW3301,PLW1510,E721,PLW0120,F811,PLC2401,PLC0414,E743,PLE0101,PLR0124,PLW0127,F541,PLW1508,PLC3002,E742,PLE0302,PLW0129,F402,PLC0208 ./src/sage/ --statistics
warning: `PLR1701` has been remapped to `SIM101`.
479 PLW0211 bad-staticmethod-argument
 69 PLR1730 if-stmt-min-max
 63 PLW0642 self-or-cls-assignment

Run from this branch

Jack: sage % ruff check --ignore PLR2004,I001,F401,E741,F821,PLR0912,PLR0913,E402,PLR0915,PLW2901,PLR5501,PLR0911,E731,F405,PLR1714,PLR1736,F403,PLR0402,PLW0603,F841,E713,PLW0602,PLR1711,E714,PLR1701,PLR1704,PLW3301,PLW1510,E721,PLW0120,F811,PLC2401,PLC0414,E743,PLE0101,PLR0124,PLW0127,F541,PLW1508,PLC3002,E742,PLE0302,PLW0129,F402,PLC0208 ./src/sage/ --statistics
warning: `PLR1701` has been remapped to `SIM101`.
479	PLW0211	bad-staticmethod-argument
 63	PLW0642	self-or-cls-assignment

Copy link

github-actions bot commented Aug 21, 2024

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

@GiacomoPope GiacomoPope added t: refactoring and removed p: CI Fix merged before running CI tests labels Aug 21, 2024
@GiacomoPope
Copy link
Contributor Author

@fchapoton I'm not sure how to best rebase this PR on top of your work in #38516 -- any Git/GitHub tips on how to do this best?

@fchapoton
Copy link
Contributor

not sure either, sorry. Fetch both branches and merge one into the other ?

@GiacomoPope
Copy link
Contributor Author

I imagine your CI fix will be merged before this, so I might just wait for this and then I'll rebase from develop

@GiacomoPope
Copy link
Contributor Author

@fchapoton This now is up to date with your changes which got merged

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, good

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.

2 participants