search

astral-sh / ruff

An extremely fast Python linter and code formatter, written in Rust.
https://docs.astral.sh/ruff
MIT License
31.4k stars 1.05k forks source link

[`flake8-simplify`] Stabilize detection of Yoda conditions for "constant" collections (`SIM300`) #12050

Closed charliermarsh closed 3 months ago

charliermarsh commented 3 months ago

Summary

See: https://github.com/astral-sh/ruff/pull/9164.

We made a bunch of improvements to the rule, to handle "constant" collections (e.g., lists that consist of constants). This will lead to new violations, but I do think it's a better behavior and it's clearly stable.

charliermarsh commented 3 months ago

A bit more controversial than the others because the scope is larger (hopefully ecosystem reflects that).

AlexWaygood commented 3 months ago

I pushed to your branch to fix some trivial clippy issues, but looks like some snapshots are out of date as well

github-actions[bot] commented 3 months ago

ruff-ecosystem results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+2403 -1905 violations, +0 -0 fixes in 6 projects; 44 projects unchanged)

apache/airflow (+1827 -1422 violations, +0 -0 fixes) 

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

+ airflow/providers/arangodb/sensors/arangodb.py:54:16: SIM300 [*] Yoda condition detected
- airflow/providers/arangodb/sensors/arangodb.py:54:16: SIM300 [*] Yoda conditions are discouraged, use `records != 0` instead
+ airflow/providers/google/cloud/sensors/dataproc.py:118:14: SIM300 [*] Yoda condition detected
- airflow/providers/google/cloud/sensors/dataproc.py:118:14: SIM300 [*] Yoda conditions are discouraged, use `state == JobStatus.State.DONE` instead
+ airflow/providers/google/cloud/sensors/dataproc.py:121:14: SIM300 [*] Yoda condition detected
- airflow/providers/google/cloud/sensors/dataproc.py:121:14: SIM300 [*] Yoda conditions are discouraged, use `state == JobStatus.State.ATTEMPT_FAILURE` instead
+ dev/breeze/src/airflow_breeze/utils/selective_checks.py:1169:17: SIM300 [*] Yoda condition detected
- dev/breeze/src/airflow_breeze/utils/selective_checks.py:1169:17: SIM300 [*] Yoda conditions are discouraged, use `label.lower() == "amd"` instead
+ dev/breeze/src/airflow_breeze/utils/selective_checks.py:1170:20: SIM300 [*] Yoda condition detected
- dev/breeze/src/airflow_breeze/utils/selective_checks.py:1170:20: SIM300 [*] Yoda conditions are discouraged, use `label.lower() == "amd64"` instead
+ dev/breeze/src/airflow_breeze/utils/selective_checks.py:1171:20: SIM300 [*] Yoda condition detected
- dev/breeze/src/airflow_breeze/utils/selective_checks.py:1171:20: SIM300 [*] Yoda conditions are discouraged, use `label.lower() == "x64"` instead
+ dev/breeze/src/airflow_breeze/utils/selective_checks.py:1172:20: SIM300 [*] Yoda condition detected
- dev/breeze/src/airflow_breeze/utils/selective_checks.py:1172:20: SIM300 [*] Yoda conditions are discouraged, use `label == "asf-runner"` instead
+ dev/breeze/src/airflow_breeze/utils/selective_checks.py:1189:17: SIM300 [*] Yoda condition detected
- dev/breeze/src/airflow_breeze/utils/selective_checks.py:1189:17: SIM300 [*] Yoda conditions are discouraged, use `label.lower() == "arm"` instead
+ dev/breeze/src/airflow_breeze/utils/selective_checks.py:1189:43: SIM300 [*] Yoda condition detected
- dev/breeze/src/airflow_breeze/utils/selective_checks.py:1189:43: SIM300 [*] Yoda conditions are discouraged, use `label.lower() == "arm64"` instead
+ dev/breeze/src/airflow_breeze/utils/selective_checks.py:1189:71: SIM300 [*] Yoda condition detected
- dev/breeze/src/airflow_breeze/utils/selective_checks.py:1189:71: SIM300 [*] Yoda conditions are discouraged, use `label == "asf-arm"` instead
+ dev/breeze/tests/test_packages.py:112:12: SIM300 [*] Yoda condition detected
+ dev/breeze/tests/test_packages.py:117:12: SIM300 [*] Yoda condition detected
+ dev/breeze/tests/test_packages.py:122:12: SIM300 [*] Yoda condition detected
+ docker_tests/test_prod_image.py:60:16: SIM300 [*] Yoda condition detected
- docker_tests/test_prod_image.py:60:16: SIM300 [*] Yoda conditions are discouraged, use `ctx.value.return_code == 2` instead
+ docker_tests/test_prod_image.py:66:16: SIM300 [*] Yoda condition detected
- docker_tests/test_prod_image.py:66:16: SIM300 [*] Yoda conditions are discouraged, use `ctx.value.return_code == 2` instead
+ helm_tests/airflow_aux/test_airflow_common.py:102:16: SIM300 [*] Yoda condition detected
- helm_tests/airflow_aux/test_airflow_common.py:102:16: SIM300 [*] Yoda conditions are discouraged, use `len(docs) == 3` instead
+ helm_tests/airflow_aux/test_airflow_common.py:167:16: SIM300 [*] Yoda condition detected
- helm_tests/airflow_aux/test_airflow_common.py:167:16: SIM300 [*] Yoda conditions are discouraged, use `len(k8s_objects) == 7` instead
+ helm_tests/airflow_aux/test_airflow_common.py:228:16: SIM300 [*] Yoda condition detected
- helm_tests/airflow_aux/test_airflow_common.py:228:16: SIM300 [*] Yoda conditions are discouraged, use `len(k8s_objects) == 12` instead
+ helm_tests/airflow_aux/test_airflow_common.py:236:20: SIM300 [*] Yoda condition detected
- helm_tests/airflow_aux/test_airflow_common.py:236:20: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_airflow_common.py:244:20: SIM300 [*] Yoda condition detected
- helm_tests/airflow_aux/test_airflow_common.py:244:20: SIM300 [*] Yoda conditions are discouraged
... 3212 additional changes omitted for project

bokeh/bokeh (+162 -149 violations, +0 -0 fixes) 

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

+ examples/topics/stats/sinaplot.py:42:19: SIM300 [*] Yoda condition detected
- examples/topics/stats/sinaplot.py:42:19: SIM300 [*] Yoda conditions are discouraged, use `month == df.MONTH` instead
+ src/bokeh/core/property/numeric.py:118:17: SIM300 [*] Yoda condition detected
- src/bokeh/core/property/numeric.py:118:17: SIM300 [*] Yoda conditions are discouraged, use `value > 0` instead
+ src/bokeh/core/property/numeric.py:68:17: SIM300 [*] Yoda condition detected
- src/bokeh/core/property/numeric.py:68:17: SIM300 [*] Yoda conditions are discouraged, use `value >= 0` instead
+ src/bokeh/core/property/numeric.py:80:17: SIM300 [*] Yoda condition detected
- src/bokeh/core/property/numeric.py:80:17: SIM300 [*] Yoda conditions are discouraged, use `value > 0` instead
+ src/bokeh/core/property/numeric.py:99:17: SIM300 [*] Yoda condition detected
- src/bokeh/core/property/numeric.py:99:17: SIM300 [*] Yoda conditions are discouraged, use `value >= 0` instead
... 301 additional changes omitted for project

demisto/content (+413 -331 violations, +0 -0 fixes) 

+ Packs/AcalvioShadowplex/Integrations/acalvioapp/acalvioapp.py:114:12: SIM300 [*] Yoda condition detected
- Packs/AcalvioShadowplex/Integrations/acalvioapp/acalvioapp.py:114:12: SIM300 [*] Yoda conditions are discouraged, use `_action != Action.TEST_CONN` instead
+ Packs/AcalvioShadowplex/Integrations/acalvioapp/acalvioapp.py:281:31: SIM300 [*] Yoda condition detected
- Packs/AcalvioShadowplex/Integrations/acalvioapp/acalvioapp.py:281:31: SIM300 [*] Yoda conditions are discouraged, use `res_json['rescode'] == 0` instead
+ Packs/AcalvioShadowplex/Integrations/acalvioapp/acalvioapp.py:314:33: SIM300 [*] Yoda condition detected
- Packs/AcalvioShadowplex/Integrations/acalvioapp/acalvioapp.py:314:33: SIM300 [*] Yoda conditions are discouraged, use `res_json['rescode'] == 0` instead
+ Packs/AcalvioShadowplex/Integrations/acalvioapp/acalvioapp.py:347:31: SIM300 [*] Yoda condition detected
- Packs/AcalvioShadowplex/Integrations/acalvioapp/acalvioapp.py:347:31: SIM300 [*] Yoda conditions are discouraged, use `res_json['rescode'] == 0` instead
+ Packs/AcalvioShadowplex/Integrations/acalvioapp/acalvioapp.py:381:33: SIM300 [*] Yoda condition detected
- Packs/AcalvioShadowplex/Integrations/acalvioapp/acalvioapp.py:381:33: SIM300 [*] Yoda conditions are discouraged, use `res_json['rescode'] == 0` instead
... 734 additional changes omitted for project

model-bakers/model_bakery (+1 -0 violations, +0 -0 fixes) 

+ tests/test_baker.py:817:16: SIM300 [*] Yoda condition detected

python-poetry/poetry (+0 -0 violations, +0 -0 fixes) 








indico/indico (+0 -3 violations, +0 -0 fixes)




- indico/web/rh_test.py:85:12: SIM300 [*] Yoda conditions are discouraged, use `{} == DummyRH._CORS` instead
- indico/web/rh_test.py:92:12: SIM300 [*] Yoda conditions are discouraged, use `{} == DummyRH._CORS` instead
- indico/web/rh_test.py:99:12: SIM300 [*] Yoda conditions are discouraged, use `{'origins': 'localhost'} == DummyRH._CORS` instead








Changes by rule (1 rules affected)



| code | total | + violation | - violation | + fix | - fix |
| ---- | ------- | --------- | -------- | ----- | ---- |
| SIM300 | 4308 | 2403 | 1905 | 0 | 0 |






Linter (preview)


ℹ️ ecosystem check detected linter changes. (+1990 -1990 violations, +0 -0 fixes in 3 projects; 1 project error; 46 projects unchanged)


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



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







+ airflow/providers/arangodb/sensors/arangodb.py:54:16: SIM300 [*] Yoda condition detected
- airflow/providers/arangodb/sensors/arangodb.py:54:16: SIM300 [*] Yoda conditions are discouraged, use `records != 0` instead
+ airflow/providers/google/cloud/sensors/dataproc.py:118:14: SIM300 [*] Yoda condition detected
- airflow/providers/google/cloud/sensors/dataproc.py:118:14: SIM300 [*] Yoda conditions are discouraged, use `state == JobStatus.State.DONE` instead
+ airflow/providers/google/cloud/sensors/dataproc.py:121:14: SIM300 [*] Yoda condition detected
- airflow/providers/google/cloud/sensors/dataproc.py:121:14: SIM300 [*] Yoda conditions are discouraged, use `state == JobStatus.State.ATTEMPT_FAILURE` instead
+ dev/breeze/src/airflow_breeze/utils/selective_checks.py:1169:17: SIM300 [*] Yoda condition detected
- dev/breeze/src/airflow_breeze/utils/selective_checks.py:1169:17: SIM300 [*] Yoda conditions are discouraged, use `label.lower() == "amd"` instead
+ dev/breeze/src/airflow_breeze/utils/selective_checks.py:1170:20: SIM300 [*] Yoda condition detected
- dev/breeze/src/airflow_breeze/utils/selective_checks.py:1170:20: SIM300 [*] Yoda conditions are discouraged, use `label.lower() == "amd64"` instead
+ dev/breeze/src/airflow_breeze/utils/selective_checks.py:1171:20: SIM300 [*] Yoda condition detected
- dev/breeze/src/airflow_breeze/utils/selective_checks.py:1171:20: SIM300 [*] Yoda conditions are discouraged, use `label.lower() == "x64"` instead
+ dev/breeze/src/airflow_breeze/utils/selective_checks.py:1172:20: SIM300 [*] Yoda condition detected
- dev/breeze/src/airflow_breeze/utils/selective_checks.py:1172:20: SIM300 [*] Yoda conditions are discouraged, use `label == "asf-runner"` instead
+ dev/breeze/src/airflow_breeze/utils/selective_checks.py:1189:17: SIM300 [*] Yoda condition detected
- dev/breeze/src/airflow_breeze/utils/selective_checks.py:1189:17: SIM300 [*] Yoda conditions are discouraged, use `label.lower() == "arm"` instead
+ dev/breeze/src/airflow_breeze/utils/selective_checks.py:1189:43: SIM300 [*] Yoda condition detected
- dev/breeze/src/airflow_breeze/utils/selective_checks.py:1189:43: SIM300 [*] Yoda conditions are discouraged, use `label.lower() == "arm64"` instead
+ dev/breeze/src/airflow_breeze/utils/selective_checks.py:1189:71: SIM300 [*] Yoda condition detected
- dev/breeze/src/airflow_breeze/utils/selective_checks.py:1189:71: SIM300 [*] Yoda conditions are discouraged, use `label == "asf-arm"` instead
+ dev/breeze/tests/test_packages.py:112:12: SIM300 [*] Yoda condition detected
- dev/breeze/tests/test_packages.py:112:12: SIM300 [*] Yoda conditions are discouraged, use `get_removed_provider_ids() == []` instead
+ dev/breeze/tests/test_packages.py:117:12: SIM300 [*] Yoda condition detected
- dev/breeze/tests/test_packages.py:117:12: SIM300 [*] Yoda conditions are discouraged, use `get_suspended_provider_ids() == []` instead
+ dev/breeze/tests/test_packages.py:122:12: SIM300 [*] Yoda condition detected
- dev/breeze/tests/test_packages.py:122:12: SIM300 [*] Yoda conditions are discouraged, use `get_suspended_provider_folders() == []` instead
+ docker_tests/test_prod_image.py:60:16: SIM300 [*] Yoda condition detected
- docker_tests/test_prod_image.py:60:16: SIM300 [*] Yoda conditions are discouraged, use `ctx.value.return_code == 2` instead
+ docker_tests/test_prod_image.py:66:16: SIM300 [*] Yoda condition detected
- docker_tests/test_prod_image.py:66:16: SIM300 [*] Yoda conditions are discouraged, use `ctx.value.return_code == 2` instead
+ helm_tests/airflow_aux/test_airflow_common.py:102:16: SIM300 [*] Yoda condition detected
- helm_tests/airflow_aux/test_airflow_common.py:102:16: SIM300 [*] Yoda conditions are discouraged, use `len(docs) == 3` instead
+ helm_tests/airflow_aux/test_airflow_common.py:167:16: SIM300 [*] Yoda condition detected
- helm_tests/airflow_aux/test_airflow_common.py:167:16: SIM300 [*] Yoda conditions are discouraged, use `len(k8s_objects) == 7` instead
+ helm_tests/airflow_aux/test_airflow_common.py:228:16: SIM300 [*] Yoda condition detected
- helm_tests/airflow_aux/test_airflow_common.py:228:16: SIM300 [*] Yoda conditions are discouraged, use `len(k8s_objects) == 12` instead
+ helm_tests/airflow_aux/test_airflow_common.py:236:20: SIM300 [*] Yoda condition detected
- helm_tests/airflow_aux/test_airflow_common.py:236:20: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_airflow_common.py:244:20: SIM300 [*] Yoda condition detected
- helm_tests/airflow_aux/test_airflow_common.py:244:20: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_airflow_common.py:245:20: SIM300 [*] Yoda condition detected
- helm_tests/airflow_aux/test_airflow_common.py:245:20: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_airflow_common.py:246:20: SIM300 [*] Yoda condition detected
- helm_tests/airflow_aux/test_airflow_common.py:246:20: SIM300 [*] Yoda conditions are discouraged
+ helm_tests/airflow_aux/test_airflow_common.py:387:16: SIM300 [*] Yoda condition detected
... 3609 additional changes omitted for project








bokeh/bokeh (+162 -162 violations, +0 -0 fixes)



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







+ examples/topics/stats/sinaplot.py:42:19: SIM300 [*] Yoda condition detected
- examples/topics/stats/sinaplot.py:42:19: SIM300 [*] Yoda conditions are discouraged, use `month == df.MONTH` instead
+ src/bokeh/core/property/numeric.py:118:17: SIM300 [*] Yoda condition detected
- src/bokeh/core/property/numeric.py:118:17: SIM300 [*] Yoda conditions are discouraged, use `value > 0` instead
+ src/bokeh/core/property/numeric.py:68:17: SIM300 [*] Yoda condition detected
- src/bokeh/core/property/numeric.py:68:17: SIM300 [*] Yoda conditions are discouraged, use `value >= 0` instead
+ src/bokeh/core/property/numeric.py:80:17: SIM300 [*] Yoda condition detected
- src/bokeh/core/property/numeric.py:80:17: SIM300 [*] Yoda conditions are discouraged, use `value > 0` instead
+ src/bokeh/core/property/numeric.py:99:17: SIM300 [*] Yoda condition detected
- src/bokeh/core/property/numeric.py:99:17: SIM300 [*] Yoda conditions are discouraged, use `value >= 0` instead
... 314 additional changes omitted for project








model-bakers/model_bakery (+1 -1 violations, +0 -0 fixes)



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







+ tests/test_baker.py:817:16: SIM300 [*] Yoda condition detected
- tests/test_baker.py:817:16: SIM300 [*] Yoda conditions are discouraged, use `instance.fk == ["foo"]` instead








demisto/content (error)



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






```
warning: The top-level linter settings are deprecated in favour of their counterparts in the `lint` section. Please update the following options in `pyproject.toml`:
  - 'ignore' -> 'lint.ignore'
  - 'select' -> 'lint.select'
  - 'unfixable' -> 'lint.unfixable'
  - 'per-file-ignores' -> 'lint.per-file-ignores'
warning: `PGH001` has been remapped to `S307`.
warning: `PGH002` has been remapped to `G010`.
warning: `PLR1701` has been remapped to `SIM101`.
ruff failed
  Cause: Selection of deprecated rule `E999` is not allowed when preview is enabled.
```






Changes by rule (1 rules affected)



| code | total | + violation | - violation | + fix | - fix |
| ---- | ------- | --------- | -------- | ----- | ---- |
| SIM300 | 3980 | 1990 | 1990 | 0 | 0 |




            

        

            

            

                MichaReiser
                commented
                 3 months ago            

            

                
It seems that the vast majority of new violations is for usages in tests, specifically inside of assert. I think the rule should be disabled for assertions because a common pattern is to write the expected value on the left, which often is a literal. 


I'm also not sure if I would count this as a yoga condition:


assert [] == get_removed_provider_ids()


The right isn't a variable, it's a function call.
            

        

            

            

                AlexWaygood
                commented
                 3 months ago            

            

                
I would always put the thing I was testing (the result of the function call) on the left, and the thing I expected it to be on the right. assert [] == my_function_call() is just as much a Yoda condition as anything else this rule is detecting, in my opinion.
            

        

            

            

                AlexWaygood
                commented
                 3 months ago            

            

                
I'm pretty strongly of the opinion that this is a clear improvement for this rule. In all of the Python codebases I've worked on, whether the test suite is using unittest or pytest, the object being tested goes on the left-hand side of the assertion. If some codebases have the opposite convention, they may just want to switch this rule off for their codebases, but in my opinion this change makes the rule more consistent and smarter without changing the basic premise of the rule. If you disagree with the premise, you can always switch it off ;-)


Pytest doesn't appear to display it any better or worse depending on the order of the assertion:


image