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

[Ruff v0.5] Stabilise 15 pylint rules #12051

Merged
merged 4 commits into from
Jun 27, 2024
Merged

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Jun 26, 2024

Summary

Stabilise the following rules:

  • bad-open-mode (PLW1501)
  • empty-comment (PLR2044)
  • global-at-module-level (PLW0604)
  • literal-membership (PLR6201)
  • misplaced-bare-raise (PLE0704)
  • nan-comparison (PLW0177)
  • non-ascii-import-name (PLC2403)
  • non-ascii-name (PLC2401)
  • nonlocal-and-global (PLE0115)
  • potential-index-error (PLE0643)
  • redeclared-assigned-name (PLW0128)
  • redefined-argument-from-locals (PLR1704)
  • repeated-keyword-argument (PLE1132)
  • super-without-brackets (PLW0245)
  • unnecessary-list-index-lookup (PLR1736)
  • useless-exception-statement (PLW0133)
  • useless-with-lock (PLW2101)

These rules have all been in preview for over 3 months; there are no open issues about them, and there haven't been issues about them for months; and their recommendations seem pretty sane and uncontroversial. For some of them, the docs or messages to the user were slightly suboptimal; I've made a few touch-ups as part of this PR.

Many other pylint rules have somewhat opinionated or controversial changes; this PR deliberately leaves quite a few pylint rules still in preview.

Test Plan

cargo test, but the ecosystem check will be the real judge...

@AlexWaygood AlexWaygood added the rule Implementing or modifying a lint rule label Jun 26, 2024
Copy link
Contributor

github-actions bot commented Jun 26, 2024

ruff-ecosystem results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+66 -4 violations, +0 -0 fixes in 11 projects; 39 projects unchanged)

DisnakeDev/disnake (+2 -0 violations, +0 -0 fixes)

+ disnake/state.py:456:13: PLW0133 Missing `raise` statement on exception
+ disnake/state.py:476:13: PLW0133 Missing `raise` statement on exception

PlasmaPy/PlasmaPy (+12 -0 violations, +0 -0 fixes)

+ src/plasmapy/analysis/nullpoint.py:29:1: PLW0604 `global` at module level is redundant
+ src/plasmapy/dispersion/analytical/two_fluid_.py:316:9: PLC2401 Variable name `ω` contains a non-ASCII character
+ src/plasmapy/dispersion/numerical/kinetic_alfven_.py:234:9: PLC2401 Variable name `θ` contains a non-ASCII character
+ src/plasmapy/formulary/radiation.py:123:5: PLC2401 Variable name `ω` contains a non-ASCII character
+ src/plasmapy/formulary/radiation.py:124:5: PLC2401 Variable name `ω_pe` contains a non-ASCII character
+ src/plasmapy/formulary/relativity.py:187:5: PLC2401 Variable name `γ` contains a non-ASCII character
+ src/plasmapy/formulary/relativity.py:496:30: PLC2401 Argument name `γ` contains a non-ASCII character
... 4 additional changes omitted for rule PLC2401
+ tests/dispersion/test_dispersion_functions.py:10:25: PLC2403 Module alias `π` contains a non-ASCII character
+ tests/dispersion/test_dispersion_functions.py:11:36: PLC2403 Module alias `Γ` contains a non-ASCII character
... 3 additional changes omitted for project

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

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

+ airflow/cli/commands/task_command.py:702:38: PLR1704 Redefining argument with the local name `session`
+ airflow/decorators/base.py:157:13: PLR1704 Redefining argument with the local name `task_id`
+ dev/breeze/src/airflow_breeze/commands/ci_image_commands.py:444:17: PLR1704 Redefining argument with the local name `platform`
+ dev/breeze/src/airflow_breeze/commands/ci_image_commands.py:459:17: PLR1704 Redefining argument with the local name `python`
+ dev/breeze/src/airflow_breeze/commands/ci_image_commands.py:646:13: PLR1704 Redefining argument with the local name `python`
+ dev/breeze/src/airflow_breeze/commands/kubernetes_commands.py:689:51: PLR1704 Redefining argument with the local name `python`
... 13 additional changes omitted for rule PLR1704
+ dev/breeze/src/airflow_breeze/utils/parallel.py:297:57: PLR1736 [*] List index lookup in `enumerate()` loop
+ dev/breeze/src/airflow_breeze/utils/parallel.py:299:40: PLR1736 [*] List index lookup in `enumerate()` loop
+ docs/exts/docroles.py:21:1: PLR2044 [*] Line with empty comment
+ docs/exts/docroles.py:22:1: PLR2044 [*] Line with empty comment
+ scripts/ci/pre_commit/check_provider_airflow_compatibility.py:51:20: PLR1736 [*] List index lookup in `enumerate()` loop
+ tests/providers/common/sql/hooks/test_sql.py:18:1: PLR2044 [*] Line with empty comment
+ tests/providers/common/sql/operators/test_sql.py:268:20: PLW0128 Redeclared variable `operator` in assignment
+ tests/providers/databricks/hooks/test_databricks_sql.py:18:1: PLR2044 [*] Line with empty comment
+ tests/providers/exasol/hooks/test_sql.py:18:1: PLR2044 [*] Line with empty comment
+ tests/providers/google/suite/transfers/test_gcs_to_gdrive.py:123:5: PLR2044 [*] Line with empty comment
... 3 additional changes omitted for rule PLR2044
... 14 additional changes omitted for project

aws/aws-sam-cli (+1 -0 violations, +0 -0 fixes)

+ samcli/local/docker/lambda_container.py:157:9: PLR2044 [*] Line with empty comment

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

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

+ src/bokeh/document/callbacks.py:361:13: PLR1704 Redefining argument with the local name `callback_obj`
+ src/bokeh/plotting/_figure.py:387:13: PLR1704 Redefining argument with the local name `kw`
+ src/bokeh/plotting/_figure.py:428:13: PLR1704 Redefining argument with the local name `kw`
+ src/bokeh/plotting/_figure.py:478:17: PLR1704 Redefining argument with the local name `kw`
+ src/bokeh/plotting/_figure.py:567:13: PLR1704 Redefining argument with the local name `kw`
+ src/bokeh/plotting/_figure.py:609:13: PLR1704 Redefining argument with the local name `kw`
+ tests/support/util/examples.py:161:9: PLR1704 Redefining argument with the local name `path`

ibis-project/ibis (+7 -0 violations, +0 -0 fixes)

+ ibis/__init__.py:126:9: PLR1704 Redefining argument with the local name `name`
+ ibis/backends/dask/helpers.py:116:13: PLR1704 Redefining argument with the local name `name`
+ ibis/backends/sql/rewrites.py:258:9: PLR1704 Redefining argument with the local name `node`
+ ibis/expr/types/joins.py:372:13: PLR1704 Redefining argument with the local name `right`
+ ibis/expr/types/relations.py:1884:13: PLR1704 Redefining argument with the local name `table`
+ ibis/tests/expr/test_struct.py:75:32: PLR1704 Redefining argument with the local name `t`
+ ibis/tests/expr/test_struct.py:75:35: PLR1704 Redefining argument with the local name `s`

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

+ pymilvus/client/types.py:136:5: PLR2044 [*] Line with empty comment

pypa/pip (+1 -0 violations, +0 -0 fixes)

+ src/pip/_internal/utils/misc.py:152:5: PLE0704 Bare `raise` statement is not inside an exception handler

zulip/zulip (+4 -0 violations, +0 -0 fixes)

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

+ zerver/lib/scim.py:232:13: PLR1704 Redefining argument with the local name `path`
+ zerver/webhooks/clubhouse/view.py:178:9: PLR1704 Redefining argument with the local name `action`
+ zerver/webhooks/clubhouse/view.py:467:13: PLR1704 Redefining argument with the local name `action`
+ zerver/webhooks/clubhouse/view.py:669:13: PLR1704 Redefining argument with the local name `action`

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

- indico/core/db/sqlalchemy/core.py:53:16: RUF100 [*] Unused `noqa` directive (non-enabled: `PLE0704`)
- indico/web/flask/errors.py:112:16: RUF100 [*] Unused `noqa` directive (non-enabled: `PLE0704`)
- indico/web/flask/errors.py:126:12: RUF100 [*] Unused `noqa` directive (non-enabled: `PLE0704`)
- indico/web/flask/errors.py:74:16: RUF100 [*] Unused `noqa` directive (non-enabled: `PLE0704`)

pytest-dev/pytest (+1 -0 violations, +0 -0 fixes)

+ src/_pytest/logging.py:402:13: PLE0704 Bare `raise` statement is not inside an exception handler

Changes by rule (10 rules affected)

code total + violation - violation + fix - fix
PLR1704 36 36 0 0 0
PLR2044 10 10 0 0 0
PLC2401 9 9 0 0 0
RUF100 4 0 4 0 0
PLR1736 3 3 0 0 0
PLW0133 2 2 0 0 0
PLC2403 2 2 0 0 0
PLE0704 2 2 0 0 0
PLW0604 1 1 0 0 0
PLW0128 1 1 0 0 0

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+852 -852 violations, +0 -0 fixes in 12 projects; 1 project error; 37 projects unchanged)

aiven/aiven-client (+1 -1 violations, +0 -0 fixes)

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

- aiven/client/argx.py:111:57: PLR6201 Use a `set` literal when testing for membership
+ aiven/client/argx.py:111:57: PLR6201 Use a set literal when testing for membership

PlasmaPy/PlasmaPy (+67 -67 violations, +0 -0 fixes)

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

- src/plasmapy/dispersion/analytical/mhd_waves_.py:121:26: PLR6201 Use a `set` literal when testing for membership
+ src/plasmapy/dispersion/analytical/mhd_waves_.py:121:26: PLR6201 Use a set literal when testing for membership
- src/plasmapy/dispersion/analytical/mhd_waves_.py:131:30: PLR6201 Use a `set` literal when testing for membership
+ src/plasmapy/dispersion/analytical/mhd_waves_.py:131:30: PLR6201 Use a set literal when testing for membership
- src/plasmapy/dispersion/analytical/stix_.py:192:24: PLR6201 Use a `set` literal when testing for membership
+ src/plasmapy/dispersion/analytical/stix_.py:192:24: PLR6201 Use a set literal when testing for membership
... 107 additional changes omitted for rule PLR6201
+ src/plasmapy/dispersion/analytical/two_fluid_.py:316:9: PLC2401 Variable name `ω` contains a non-ASCII character
- src/plasmapy/dispersion/analytical/two_fluid_.py:316:9: PLC2401 Variable name `ω` contains a non-ASCII character, consider renaming it
+ src/plasmapy/dispersion/numerical/kinetic_alfven_.py:234:9: PLC2401 Variable name `θ` contains a non-ASCII character
- src/plasmapy/dispersion/numerical/kinetic_alfven_.py:234:9: PLC2401 Variable name `θ` contains a non-ASCII character, consider renaming it
... 124 additional changes omitted for project

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

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

- airflow/__init__.py:98:65: PLR6201 Use a `set` literal when testing for membership
+ airflow/__init__.py:98:65: PLR6201 Use a set literal when testing for membership
- airflow/__main__.py:49:31: PLR6201 Use a `set` literal when testing for membership
+ airflow/__main__.py:49:31: PLR6201 Use a set literal when testing for membership
- airflow/__main__.py:56:31: PLR6201 Use a `set` literal when testing for membership
+ airflow/__main__.py:56:31: PLR6201 Use a set literal when testing for membership
- airflow/api_connexion/endpoints/task_instance_endpoint.py:277:26: PLR6201 Use a `set` literal when testing for membership
+ airflow/api_connexion/endpoints/task_instance_endpoint.py:277:26: PLR6201 Use a set literal when testing for membership
- airflow/api_connexion/endpoints/task_instance_endpoint.py:729:20: PLR6201 Use a `set` literal when testing for membership
+ airflow/api_connexion/endpoints/task_instance_endpoint.py:729:20: PLR6201 Use a set literal when testing for membership
... 639 additional changes omitted for rule PLR6201
+ dev/breeze/src/airflow_breeze/utils/parallel.py:297:57: PLR1736 [*] List index lookup in `enumerate()` loop
- dev/breeze/src/airflow_breeze/utils/parallel.py:297:57: PLR1736 [*] Unnecessary lookup of list item by index
+ dev/breeze/src/airflow_breeze/utils/parallel.py:299:40: PLR1736 [*] List index lookup in `enumerate()` loop
- dev/breeze/src/airflow_breeze/utils/parallel.py:299:40: PLR1736 [*] Unnecessary lookup of list item by index
+ scripts/ci/pre_commit/check_provider_airflow_compatibility.py:51:20: PLR1736 [*] List index lookup in `enumerate()` loop
- scripts/ci/pre_commit/check_provider_airflow_compatibility.py:51:20: PLR1736 [*] Unnecessary lookup of list item by index
... 638 additional changes omitted for project

aws/aws-sam-cli (+42 -42 violations, +0 -0 fixes)

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

- samcli/cli/main.py:89:27: PLR6201 Use a `set` literal when testing for membership
+ samcli/cli/main.py:89:27: PLR6201 Use a set literal when testing for membership
- samcli/cli/types.py:58:56: PLR6201 Use a `set` literal when testing for membership
+ samcli/cli/types.py:58:56: PLR6201 Use a set literal when testing for membership
- samcli/cli/types.py:58:84: PLR6201 Use a `set` literal when testing for membership
+ samcli/cli/types.py:58:84: PLR6201 Use a set literal when testing for membership
- samcli/commands/_utils/custom_options/hook_name_option.py:46:28: PLR6201 Use a `set` literal when testing for membership
+ samcli/commands/_utils/custom_options/hook_name_option.py:46:28: PLR6201 Use a set literal when testing for membership
- samcli/commands/_utils/template.py:162:34: PLR6201 Use a `set` literal when testing for membership
+ samcli/commands/_utils/template.py:162:34: PLR6201 Use a set literal when testing for membership
... 74 additional changes omitted for project

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

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

- examples/output/apis/file_html.py:39:48: PLR6201 Use a `set` literal when testing for membership
+ examples/output/apis/file_html.py:39:48: PLR6201 Use a set literal when testing for membership
- examples/plotting/sprint.py:23:48: PLR6201 Use a `set` literal when testing for membership
+ examples/plotting/sprint.py:23:48: PLR6201 Use a set literal when testing for membership
- release/checks.py:69:30: PLR6201 Use a `set` literal when testing for membership
+ release/checks.py:69:30: PLR6201 Use a set literal when testing for membership
- src/bokeh/__init__.py:104:24: PLR6201 Use a `set` literal when testing for membership
+ src/bokeh/__init__.py:104:24: PLR6201 Use a set literal when testing for membership
- src/bokeh/command/subcommands/__init__.py:56:48: PLR6201 Use a `set` literal when testing for membership
+ src/bokeh/command/subcommands/__init__.py:56:48: PLR6201 Use a set literal when testing for membership
... 46 additional changes omitted for project

freedomofpress/securedrop (+40 -40 violations, +0 -0 fixes)

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

- devops/scripts/verify-mo.py:140:35: PLR6201 Use a `set` literal when testing for membership
+ devops/scripts/verify-mo.py:140:35: PLR6201 Use a set literal when testing for membership
- molecule/ansible-config/tests/test_play_configuration.py:34:25: PLR6201 Use a `set` literal when testing for membership
+ molecule/ansible-config/tests/test_play_configuration.py:34:25: PLR6201 Use a set literal when testing for membership
- securedrop/manage.py:170:22: PLR6201 Use a `set` literal when testing for membership
+ securedrop/manage.py:170:22: PLR6201 Use a set literal when testing for membership
- securedrop/manage.py:172:24: PLR6201 Use a `set` literal when testing for membership
+ securedrop/manage.py:172:24: PLR6201 Use a set literal when testing for membership
- securedrop/pretty_bad_protocol/_parsers.py:1057:25: PLR6201 Use a `set` literal when testing for membership
+ securedrop/pretty_bad_protocol/_parsers.py:1057:25: PLR6201 Use a set literal when testing for membership
... 70 additional changes omitted for project

fronzbot/blinkpy (+3 -3 violations, +0 -0 fixes)

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

- blinkpy/auth.py:150:35: PLR6201 Use a `set` literal when testing for membership
+ blinkpy/auth.py:150:35: PLR6201 Use a set literal when testing for membership
- blinkpy/camera.py:144:41: PLR6201 Use a `set` literal when testing for membership
+ blinkpy/camera.py:144:41: PLR6201 Use a set literal when testing for membership
- blinkpy/camera.py:157:25: PLR6201 Use a `set` literal when testing for membership
+ blinkpy/camera.py:157:25: PLR6201 Use a set literal when testing for membership

ibis-project/ibis (+61 -61 violations, +0 -0 fixes)

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

- ibis/backends/__init__.py:1378:18: PLR6201 Use a `set` literal when testing for membership
+ ibis/backends/__init__.py:1378:18: PLR6201 Use a set literal when testing for membership
- ibis/backends/bigquery/compiler.py:609:23: PLR6201 Use a `set` literal when testing for membership
+ ibis/backends/bigquery/compiler.py:609:23: PLR6201 Use a set literal when testing for membership
- ibis/backends/bigquery/compiler.py:635:23: PLR6201 Use a `set` literal when testing for membership
+ ibis/backends/bigquery/compiler.py:635:23: PLR6201 Use a set literal when testing for membership
- ibis/backends/bigquery/tests/system/test_client.py:308:22: PLR6201 Use a `set` literal when testing for membership
+ ibis/backends/bigquery/tests/system/test_client.py:308:22: PLR6201 Use a set literal when testing for membership
- ibis/backends/clickhouse/compiler.py:269:32: PLR6201 Use a `set` literal when testing for membership
+ ibis/backends/clickhouse/compiler.py:269:32: PLR6201 Use a set literal when testing for membership
... 112 additional changes omitted for project

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

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

- pymilvus/bulk_writer/remote_bulk_writer.py:282:31: PLR6201 Use a `set` literal when testing for membership
+ pymilvus/bulk_writer/remote_bulk_writer.py:282:31: PLR6201 Use a set literal when testing for membership
- pymilvus/client/abstract.py:443:25: PLR6201 Use a `set` literal when testing for membership
+ pymilvus/client/abstract.py:443:25: PLR6201 Use a set literal when testing for membership
- pymilvus/client/abstract.py:553:39: PLR6201 Use a `set` literal when testing for membership
+ pymilvus/client/abstract.py:553:39: PLR6201 Use a set literal when testing for membership
- pymilvus/client/abstract.py:560:43: PLR6201 Use a `set` literal when testing for membership
+ pymilvus/client/abstract.py:560:43: PLR6201 Use a set literal when testing for membership
- pymilvus/client/abstract.py:562:45: PLR6201 Use a `set` literal when testing for membership
+ pymilvus/client/abstract.py:562:45: PLR6201 Use a set literal when testing for membership
... 74 additional changes omitted for project

... Truncated remaining completed project reports due to GitHub comment length restrictions

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 (4 rules affected)

code total + violation - violation + fix - fix
PLR6201 1676 838 838 0 0
PLC2401 18 9 9 0 0
PLR1736 6 3 3 0 0
PLC2403 4 2 2 0 0

@AlexWaygood AlexWaygood changed the title Stabilise 17 pylint rules [Ruff v0.5] Stabilise 17 pylint rules Jun 26, 2024
@airreality
Copy link

Are you sure that PLR6201 is really useful rule? I believe initializing set for only one operation is too expensive and has no sense. I would prefer using tuple. And if the reason is to avoid duplicating items maybe it's better to have the rule which checks such items instead?
I see there are a lot of violations in ecosystem. The same in my repoes. I guess most of users will add this rule in ignores.

@AlexWaygood
Copy link
Member Author

Thanks @airreality. While I think it's a useful rule, and probably one that I would enable on my own projects, I agree that there's a lot of ecosystem hits for PLR6201. The performance pros and cons have already been discussed in some depth in #8758 and #9604; however, I missed that #8322 regarding this rule is still open. I think it'd be inappropriate to stabilise a rule while there's still open issues about its behaviour, so I'll axe that rule from the list.

I'm also slightly concerned about the number of hits for nan-comparison (PLW0177) on pandas, as that's a project I'd expect to follow best practices for this kind of thing. In the interests of getting the release out on schedule, I'll also axe that from the list of stabilisations for now, and I'll do an investigation later to see if those are false positives or true positives.

@AlexWaygood AlexWaygood changed the title [Ruff v0.5] Stabilise 17 pylint rules [Ruff v0.5] Stabilise 15 pylint rules Jun 27, 2024
@AlexWaygood AlexWaygood merged commit 79f815f into ruff-0.5 Jun 27, 2024
20 checks passed
@AlexWaygood AlexWaygood deleted the pylint-stabilisations branch June 27, 2024 10:24
@MichaReiser MichaReiser mentioned this pull request Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants