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

style: Fixes if-stmt-min-max (PLR1730) #3950

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

echoix
Copy link
Member

@echoix echoix commented Jun 30, 2024

Concerns Pylint rules "consider-using-min-builtin / R1730" and "consider-using-max-builtin / R1731"

Using ruff check --output-format=concise --select PLR1730 --preview --fix.

Part of the effort to introduce Pylint 3.x for #3921

Uses the fixes provided for ruff rule if-stmt-min-max (PLR1730) to fix part of Pylint's "consider-using-min-builtin / R1730" and "consider-using-max-builtin / R1731".

It also happens to reduce the number of branches and number of lines, where multiple places still exceeds some (already raised) limits, so its a good thing.

Concerns Pylint rules "consider-using-min-builtin / R1730" and "consider-using-max-builtin / R1731"

Using `ruff check --output-format=concise --select PLR1730 --preview --fix`.
@github-actions github-actions bot added GUI wxGUI related raster Related to raster data processing temporal Related to temporal data processing Python Related code is in Python libraries module display notebook labels Jun 30, 2024
@echoix echoix added this to the 8.5.0 milestone Jun 30, 2024
Copy link
Member

@ninsbl ninsbl left a comment

Choose a reason for hiding this comment

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

Nice, do you plan to apply such improvement to addons too?

@echoix
Copy link
Member Author

echoix commented Jun 30, 2024

If it is wanted, I could. But the goal now is to have less than the couple thousand Pylint violations only for python/grass and gui/wxpython.

I made sure to leave the command used in each commit/PR. So it's really a matter of milliseconds to fix. What is longer is my manual review that I do, file by file, before committing. The PRs I submitted one after the other were the accumulation of this morning and half the afternoon. It's all from one branch, then I cherry-pick individual changed to new branches created from main, hoping that the changes were completely independent to not have any conflicts. Then I wrote the PRs.

In between the changes, I review what changed in the Pylint run, and find a new issue that ruff can easily fix.

@ninsbl
Copy link
Member

ninsbl commented Jul 1, 2024

Great!

The addons repo would benefit from such an effort for sure, and IMHO code style in the core and addons repo should be as similar as possible...

But of course I understand that it is a matter of priorities...

@echoix
Copy link
Member Author

echoix commented Jul 1, 2024

I did a little bit the other weekend on the addons-repo, as I was blocked without any reviews here, and their pre-commit config is in a better shape than here. There is still OSGeo/grass-addons#1134 that is waiting for discussions.

If you were talking about this specific fix, you can apply ruff check --output-format=concise --select PLR1730 --preview --fix right away on that repo.

@echoix echoix enabled auto-merge (squash) July 1, 2024 00:26
@echoix echoix merged commit ce1a43c into OSGeo:main Jul 1, 2024
26 checks passed
@echoix echoix deleted the fix-if-stmt-min-max branch July 1, 2024 00:26
cyliang368 pushed a commit to cyliang368/grass that referenced this pull request Jul 1, 2024
Concerns Pylint rules "consider-using-min-builtin / R1730" and "consider-using-max-builtin / R1731"

Using `ruff check --output-format=concise --select PLR1730 --preview --fix`.
@a0x8o a0x8o mentioned this pull request Jul 1, 2024
a0x8o pushed a commit to a0x8o/grass that referenced this pull request Jul 2, 2024
Concerns Pylint rules "consider-using-min-builtin / R1730" and "consider-using-max-builtin / R1731"

Using `ruff check --output-format=concise --select PLR1730 --preview --fix`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
display GUI wxGUI related libraries module notebook Python Related code is in Python raster Related to raster data processing temporal Related to temporal data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants