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

[pylint] Implement if-stmt-min-max (PLR1730, PLR1731) #10002

Merged
merged 10 commits into from
Apr 6, 2024

Conversation

tibor-reiss
Copy link
Contributor

@tibor-reiss tibor-reiss commented Feb 15, 2024

Add rule consider-using-min-builtin (R1730) and consider-using-max-builtin (R1731)

See #970 for rules

Test Plan: cargo test

Copy link
Contributor

github-actions bot commented Feb 15, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+77 -70 violations, +0 -0 fixes in 4 projects; 40 projects unchanged)

apache/airflow (+3 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ airflow/cli/commands/kubernetes_command.py:84:5: PLR1730 [*] Replace `if` statement with `min_pending_minutes = min(min_pending_minutes, 5)`
+ airflow/models/taskinstance.py:2167:13: PLR1730 [*] Replace `if` statement with `min_backoff = min(min_backoff, 1)`
+ tests/cluster_policies/__init__.py:103:5: PLR1730 [*] Replace `if` statement with `max` call

milvus-io/pymilvus (+3 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ pymilvus/bulk_writer/buffer.py:234:13: PLR1730 [*] Replace `if` statement with `min` call
+ pymilvus/bulk_writer/buffer.py:236:13: PLR1730 [*] Replace `if` statement with `max` call
+ pymilvus/orm/iterator.py:54:9: PLR1730 [*] Replace `if` statement with `max` call

pandas-dev/pandas (+70 -70 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ pandas/tests/arithmetic/test_numeric.py:1000:9: PLR6301 Method `test_frame_operators_none_to_nan` could be a function, class method, or static method
- pandas/tests/arithmetic/test_numeric.py:1002:9: PLR6301 Method `test_frame_operators_none_to_nan` could be a function, class method, or static method
+ pandas/tests/arithmetic/test_numeric.py:1005:9: PLR6301 Method `test_frame_operators_empty_like` could be a function, class method, or static method
- pandas/tests/arithmetic/test_numeric.py:1007:9: PLR6301 Method `test_frame_operators_empty_like` could be a function, class method, or static method
+ pandas/tests/arithmetic/test_numeric.py:1021:9: PLR6301 Method `test_series_operators_arithmetic` could be a function, class method, or static method
- pandas/tests/arithmetic/test_numeric.py:1023:9: PLR6301 Method `test_series_operators_arithmetic` could be a function, class method, or static method
+ pandas/tests/arithmetic/test_numeric.py:1034:9: PLR6301 Method `test_series_operators_compare` could be a function, class method, or static method
- pandas/tests/arithmetic/test_numeric.py:1036:9: PLR6301 Method `test_series_operators_compare` could be a function, class method, or static method
+ pandas/tests/arithmetic/test_numeric.py:1049:9: PLR6301 Method `test_divmod` could be a function, class method, or static method
- pandas/tests/arithmetic/test_numeric.py:1051:9: PLR6301 Method `test_divmod` could be a function, class method, or static method
+ pandas/tests/arithmetic/test_numeric.py:1077:9: PLR6301 Method `test_series_divmod_zero` could be a function, class method, or static method
- pandas/tests/arithmetic/test_numeric.py:1079:9: PLR6301 Method `test_series_divmod_zero` could be a function, class method, or static method
+ pandas/tests/arithmetic/test_numeric.py:1103:9: PLR6301 Method `test_ufunc_compat` could be a function, class method, or static method
- pandas/tests/arithmetic/test_numeric.py:1105:9: PLR6301 Method `test_ufunc_compat` could be a function, class method, or static method
+ pandas/tests/arithmetic/test_numeric.py:1118:9: PLR6301 Method `test_ufunc_coercions` could be a function, class method, or static method
- pandas/tests/arithmetic/test_numeric.py:1120:9: PLR6301 Method `test_ufunc_coercions` could be a function, class method, or static method
+ pandas/tests/arithmetic/test_numeric.py:1162:9: PLR6301 Method `test_ufunc_multiple_return_values` could be a function, class method, or static method
- pandas/tests/arithmetic/test_numeric.py:1164:9: PLR6301 Method `test_ufunc_multiple_return_values` could be a function, class method, or static method
+ pandas/tests/arithmetic/test_numeric.py:1173:9: PLR6301 Method `test_ufunc_at` could be a function, class method, or static method
- pandas/tests/arithmetic/test_numeric.py:1175:9: PLR6301 Method `test_ufunc_at` could be a function, class method, or static method
+ pandas/tests/arithmetic/test_numeric.py:1184:9: PLR6301 Method `test_numarr_with_dtype_add_nan` could be a function, class method, or static method
- pandas/tests/arithmetic/test_numeric.py:1186:9: PLR6301 Method `test_numarr_with_dtype_add_nan` could be a function, class method, or static method
+ pandas/tests/arithmetic/test_numeric.py:1199:9: PLR6301 Method `test_numarr_with_dtype_add_int` could be a function, class method, or static method
- pandas/tests/arithmetic/test_numeric.py:1201:9: PLR6301 Method `test_numarr_with_dtype_add_int` could be a function, class method, or static method
... 113 additional changes omitted for rule PLR6301
+ pandas/tests/arithmetic/test_numeric.py:1491:18: PLR6201 Use a `set` literal when testing for membership
- pandas/tests/arithmetic/test_numeric.py:1493:18: PLR6201 Use a `set` literal when testing for membership
+ pandas/tests/arithmetic/test_numeric.py:1493:19: PLR6201 Use a `set` literal when testing for membership
- pandas/tests/arithmetic/test_numeric.py:1495:19: PLR6201 Use a `set` literal when testing for membership
... 112 additional changes omitted for project

rotki/rotki (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ rotkehlchen/exchanges/bybit.py:403:13: PLR1730 [*] Replace `if` statement with `lower_ts = min(lower_ts, start_ts)`

Changes by rule (3 rules affected)

code total + violation - violation + fix - fix
PLR6301 136 68 68 0 0
PLR1730 7 7 0 0 0
PLR6201 4 2 2 0 0

@tibor-reiss
Copy link
Contributor Author

tibor-reiss commented Feb 15, 2024

Hi @charliermarsh, some questions regarding this PR:

  • min and max are 2 separate rules in pylint, so I implemented as such with their respective codes - looking at other similar rules this seems to be the standard for now. However, there is minimal difference between the 2 functions min_instead_of_if and max_instead_of_if. So I wonder if it would not be better to merge these 2 rules into one?
    - is there some simple general way of matching the "value" part in literals, i.e. ignoring the range field? E.g. I could not find any example so far for a simple comparison of two python constants.

@sbrugman
Copy link
Contributor

sbrugman commented Feb 16, 2024

@tibor-reiss
Copy link
Contributor Author

@sbrugman While they look indeed similar, the implementation is quite different: FURB136 is for if expressions (in ruff it is ExprIfExp which is an Expr enum), whereas PLR1730 operates on if statements (in ruff it is StmtIf which is a Stmt enum).

@sbrugman
Copy link
Contributor

sbrugman commented Feb 17, 2024

I linked it as reference for the first point: here it is implemented as a single rule. Consistency with the upstream (mapping existing rules) takes precedence over internal consistency (merging rules that are minimally different) I suppose, so just for reference.

Nice work!

@tibor-reiss
Copy link
Contributor Author

@sbrugman Thanks for clarifying, I misunderstood your original comment. I added now a commit which merges the 2 rules - I personally also prefer this over the duplication. @charliermarsh to decide if this is acceptable - i.e. dropping PLR1731 and just using PLR1730 for both min and max.

@sbrugman
Copy link
Contributor

Sorry for the confusion, but I intended to say that the linked rule merged them at one, but since that pylint treats them as two for now the project prefers to match pylint (i.e. two rules). Anyway, indeed let the team decide on that

@tibor-reiss
Copy link
Contributor Author

@sbrugman From the linked reference I found out that there is ComparableExpr - code looks a lot cleaner now. So thanks a lot for taking a look!

@MichaReiser MichaReiser added the accepted Ready for implementation label Apr 5, 2024
@MichaReiser
Copy link
Member

MichaReiser commented Apr 5, 2024

The rule itself fits into ruff. It is similar to if-expr-min-max but we have other rules that capture the same problem but differ in the syntax.

I think we should align the name with if-expr-min-max -> if-stmt-min-max.

One thing that needs to be decided if this should be a single rule or if it's better to have two rules (to match py lint)

@charliermarsh
Copy link
Member

I'm comfortable with a single rule here. Just seems unlikely that anyone would enable one but not the other, and it better aligns with if-expr-min-max. I'll rename and merge.

@charliermarsh charliermarsh self-assigned this Apr 6, 2024
@charliermarsh charliermarsh added the preview Related to preview mode features label Apr 6, 2024
@charliermarsh charliermarsh changed the title Implement consider-using-max/min-builtin (PLR1730, PLR1731) [pylint] Implement if-stmt-min-max (PLR1730, PLR1731) Apr 6, 2024
@charliermarsh charliermarsh enabled auto-merge (squash) April 6, 2024 17:25
@charliermarsh charliermarsh merged commit 3194f90 into astral-sh:main Apr 6, 2024
17 checks passed
@dhruvmanila dhruvmanila added the rule Implementing or modifying a lint rule label Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Ready for implementation preview Related to preview mode features rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants