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

[pep8_naming] Add fixes N804 and N805 #10215

Merged
merged 9 commits into from
Mar 4, 2024

Conversation

GtrMo
Copy link
Contributor

@GtrMo GtrMo commented Mar 3, 2024

Summary

This PR fixes for invalid-first-argument rules.
The fixes rename the first argument of methods and class methods to the valid one. References to this argument are also renamed.
Fixes are skipped when another argument is named as the valid first argument.
The fix is marked as unsafe due

The functions for the N804 and N805 rules are now merged, as they only differ by the name of the valid first argument.
The rules were moved from the AST iteration to the deferred scopes to be in the function scope while creating the fix.

Test Plan

cargo test

Copy link
Contributor

github-actions bot commented Mar 3, 2024

ruff-ecosystem results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+12 -12 violations, +0 -0 fixes in 2 projects; 41 projects unchanged)

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

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

+ airflow/models/connection.py:352:18: ANN101 Missing type annotation for `cls` in method
- airflow/models/connection.py:352:18: ANN101 Missing type annotation for `cls` in method
+ airflow/models/connection.py:384:15: ANN101 Missing type annotation for `cls` in method
- airflow/models/connection.py:384:15: ANN101 Missing type annotation for `cls` in method
+ airflow/models/taskinstance.py:1454:20: ANN101 Missing type annotation for `cls` in method
- airflow/models/taskinstance.py:1454:20: ANN101 Missing type annotation for `cls` in method
+ airflow/models/variable.py:96:13: ANN101 Missing type annotation for `cls` in method
- airflow/models/variable.py:96:13: ANN101 Missing type annotation for `cls` in method
+ tests/providers/dbt/cloud/hooks/test_dbt.py:519:38: ANN101 Missing type annotation for `hook` in method
- tests/providers/dbt/cloud/hooks/test_dbt.py:519:38: ANN101 Missing type annotation for `hook` in method
+ tests/providers/odbc/hooks/test_odbc.py:70:31: ANN101 Missing type annotation for `self` in method
- tests/providers/odbc/hooks/test_odbc.py:70:31: ANN101 Missing type annotation for `self` in method

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

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

+ src/bokeh/colors/util.py:53:17: ANN101 Missing type annotation for `self` in method
- src/bokeh/colors/util.py:53:17: ANN101 Missing type annotation for `self` in method
+ src/bokeh/colors/util.py:56:21: ANN101 Missing type annotation for `self` in method
- src/bokeh/colors/util.py:56:21: ANN101 Missing type annotation for `self` in method
+ src/bokeh/colors/util.py:68:18: ANN101 Missing type annotation for `self` in method
- src/bokeh/colors/util.py:68:18: ANN101 Missing type annotation for `self` in method
+ src/bokeh/colors/util.py:72:21: ANN101 Missing type annotation for `self` in method
- src/bokeh/colors/util.py:72:21: ANN101 Missing type annotation for `self` in method
- tests/unit/bokeh/test_transform.py:65:20: N805 First argument of a method should be named `self`
+ tests/unit/bokeh/test_transform.py:65:20: N805 First argument of a method should be named `self`
- tests/unit/bokeh/test_transform.py:72:27: N805 First argument of a method should be named `self`
+ tests/unit/bokeh/test_transform.py:72:27: N805 First argument of a method should be named `self`

Changes by rule (2 rules affected)

code total + violation - violation + fix - fix
ANN101 20 10 10 0 0
N805 4 2 2 0 0

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+3 -3 violations, +0 -0 fixes in 2 projects; 41 projects unchanged)

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

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

+ tests/unit/bokeh/test_transform.py:65:20: A002 Argument `object` is shadowing a Python builtin
- tests/unit/bokeh/test_transform.py:65:20: A002 Argument `object` is shadowing a Python builtin
+ tests/unit/bokeh/test_transform.py:72:27: A002 Argument `object` is shadowing a Python builtin
- tests/unit/bokeh/test_transform.py:72:27: A002 Argument `object` is shadowing a Python builtin

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

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

+ zerver/models/realms.py:1:1: D100 Missing docstring in public module
- zerver/models/realms.py:1:1: D100 Missing docstring in public module

Changes by rule (2 rules affected)

code total + violation - violation + fix - fix
A002 4 2 2 0 0
D100 2 1 1 0 0

let binding = scope
.get_all(&first_parameter.name)
.map(|id| semantic.binding(id))
.find(|b| b.kind.is_argument())?;
Copy link
Member

Choose a reason for hiding this comment

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

Can you see if Renamer::rename would be useful here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamer::rename would rename all first_parameter.name bindings. I wanted to only renaming references to the argument binding, like this:

114 |- def bad_method(this):
115 |- this = this
114 |+ def bad_method(self):
115 |+ this = self
116 116 | this

But I didn't know the Renamer existed, and from what I've tested Pyright lsp also renames with the same logic as the Renamer. I've updated the PR to use it.

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

This is nice work, great job figuring this out.

@charliermarsh charliermarsh added the fixes Related to suggested fixes for violations label Mar 4, 2024
@charliermarsh charliermarsh enabled auto-merge (squash) March 4, 2024 02:16
@charliermarsh charliermarsh merged commit 4eac9ba into astral-sh:main Mar 4, 2024
17 checks passed
@GtrMo GtrMo deleted the fix_invalid_first_argument branch March 4, 2024 09:42
nkxxll pushed a commit to nkxxll/ruff that referenced this pull request Mar 10, 2024
## Summary

This PR fixes for `invalid-first-argument` rules.
The fixes rename the first argument of methods and class methods to the
valid one. References to this argument are also renamed.
Fixes are skipped when another argument is named as the valid first
argument.
The fix is marked as unsafe due

The functions for the `N804` and `N805` rules are now merged, as they
only differ by the name of the valid first argument.
The rules were moved from the AST iteration to the deferred scopes to be
in the function scope while creating the fix.

## Test Plan

`cargo test`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixes Related to suggested fixes for violations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants