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] Implement incorrectly-parenthesized-tuple-in-subscript (RUF031) #12480

Merged
merged 35 commits into from
Aug 7, 2024

Conversation

dylwil3
Copy link
Contributor

@dylwil3 dylwil3 commented Jul 23, 2024

Summary

Implements the new fixable lint rule RUF031 which checks for the use or omission of parentheses around tuples in subscripts, depending on the setting lint.ruff.parenthesize-tuple-in-subscript. By default, the use of parentheses is considered a violation.

For example, if d is a dictionary then

d[(0,1)]

becomes

d[0,1]

This closes #11990 and introduces configurations/options/settings for RUF rules.

Test Plan

Snapshot verified, cargo test passed, docs generated and checked that rule appeared.

@dylwil3
Copy link
Contributor Author

dylwil3 commented Jul 23, 2024

I'm not super happy with the names or descriptions for this rule and its structs/module/function/etc., so if folks have better ideas I'm all ears.

@dylwil3 dylwil3 marked this pull request as draft July 24, 2024 05:17
@dylwil3 dylwil3 marked this pull request as ready for review July 24, 2024 05:20
@MichaReiser MichaReiser added the rule Implementing or modifying a lint rule label Jul 24, 2024
@MichaReiser
Copy link
Member

Thanks for your contribution.

@AlexWaygood what's your take on this rule? I'm somewhat concerned of adding the rule to Ruff's rule set because it is a restriction lint rule, it forbids the use of specific syntax, and not a lint rule that improves the quality of the code overall.

@MichaReiser MichaReiser added the needs-decision Awaiting a decision from a maintainer label Jul 24, 2024
@AlexWaygood
Copy link
Member

I'm somewhat concerned of adding the rule to Ruff's rule set because it is a restriction lint rule, it forbids the use of specific syntax, and not a lint rule that improves the quality of the code overall.

Hmm. I'm not sure I'd call this a "restriction" rule since it doesn't forbid the user from using any Python idioms that they'd otherwise be able to use. The recommendations the rule makes are purely cosmetic; the code's AST doesn't change at all if you follow the rule's recommendations. I'd call it a formatting rule, similar to many of our pycodestyle rules.

This rule is quite opinionated, but I can definitely see that it's useful to enforce either one style or the other. The question is what style is preferable? The code becomes less noisy without the parens, but I'm not sure it's more readable -- it requires additional knowledge of Python's semantics to know that there's an implicit tuple formed by the comma there. So I think I'd support the rule being added, but I'd make it configurable (similar to e.g. our flake8-pytest-style rules) -- so you can either enforce that there are never parentheses, or that there are always parentheses.

@dylwil3
Copy link
Contributor Author

dylwil3 commented Aug 2, 2024

Great! A good opportunity to learn about rule-specific configurations. I'll make the changes.

@dylwil3 dylwil3 marked this pull request as draft August 2, 2024 22:00
Copy link
Contributor

github-actions bot commented Aug 3, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+126 -259 violations, +0 -0 fixes in 10 projects; 44 projects unchanged)

DisnakeDev/disnake (+3 -1 violations, +0 -0 fixes)

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

- disnake/ext/commands/slash_core.py:39:11: RUF022 [*] `__all__` is not sorted
+ disnake/ui/modal.py:236:22: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ disnake/ui/view.py:425:35: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ disnake/ui/view.py:530:29: RUF031 [*] Avoid parentheses for tuples in subscripts.

RasaHQ/rasa (+2 -0 violations, +0 -0 fixes)

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

+ rasa/nlu/featurizers/sparse_featurizer/lexical_syntactic_featurizer.py:345:25: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ tests/utils/test_train_utils.py:117:67: RUF031 [*] Avoid parentheses for tuples in subscripts.

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

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

+ airflow/configuration.py:730:42: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ airflow/configuration.py:758:34: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ airflow/configuration.py:793:34: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ airflow/configuration.py:980:69: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ airflow/jobs/scheduler_job_runner.py:120:43: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ airflow/jobs/scheduler_job_runner.py:539:29: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ airflow/jobs/scheduler_job_runner.py:559:29: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ airflow/jobs/scheduler_job_runner.py:599:54: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ airflow/jobs/scheduler_job_runner.py:601:21: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ airflow/migrations/versions/0046_1_10_5_change_datetime_to_datetime2_6_on_mssql_.py:276:25: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ airflow/migrations/versions/0060_2_0_0_remove_id_column_from_xcom.py:67:25: RUF031 [*] Avoid parentheses for tuples in subscripts.
... 77 additional changes omitted for project

apache/superset (+5 -108 violations, +0 -0 fixes)

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

+ scripts/permissions_cleanup.py:32:19: RUF031 [*] Avoid parentheses for tuples in subscripts.
- superset/extensions/metadb.py:101:35: ARG002 Unused method argument: `url`
- superset/extensions/metadb.py:102:9: D200 One-line docstring should fit on one line
- superset/extensions/metadb.py:102:9: D212 [*] Multi-line docstring summary should start at the first line
- superset/extensions/metadb.py:102:9: D401 First line of docstring should be in imperative mood: "A custom Shillelagh SQLAlchemy dialect with a single adapter configured."
- superset/extensions/metadb.py:105:9: DOC201 `return` is not documented in docstring
- superset/extensions/metadb.py:114:22: COM812 [*] Trailing comma missing
- superset/extensions/metadb.py:126:5: D200 One-line docstring should fit on one line
- superset/extensions/metadb.py:126:5: D212 [*] Multi-line docstring summary should start at the first line
- superset/extensions/metadb.py:126:5: D401 First line of docstring should be in imperative mood: "Decorator that prevents DML against databases where it's not allowed."
- superset/extensions/metadb.py:131:57: ANN401 Dynamically typed expressions (typing.Any) are disallowed in `*args`
- superset/extensions/metadb.py:131:72: ANN401 Dynamically typed expressions (typing.Any) are disallowed in `**kwargs`
- superset/extensions/metadb.py:131:80: ANN401 Dynamically typed expressions (typing.Any) are disallowed in `wrapper`
- superset/extensions/metadb.py:134:19: TRY003 Avoid specifying long messages outside the exception class
... 99 additional changes omitted for project

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

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

+ examples/topics/graph/node_and_edge_attributes.py:21:16: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ src/bokeh/sampledata/unemployment.py:82:18: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ src/bokeh/sampledata/us_counties.py:114:18: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ tests/unit/bokeh/colors/test_util__colors.py:141:24: RUF031 [*] Avoid parentheses for tuples in subscripts.

python/typeshed (+0 -38 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select E,F,FA,I,PYI,RUF,UP,W

- stdlib/tkinter/__init__.pyi:197:16: PYI052 Need type annotation for `Activate`
- stdlib/tkinter/__init__.pyi:198:19: PYI052 Need type annotation for `ButtonPress`
- stdlib/tkinter/__init__.pyi:200:21: PYI052 Need type annotation for `ButtonRelease`
- stdlib/tkinter/__init__.pyi:201:17: PYI052 Need type annotation for `Circulate`
- stdlib/tkinter/__init__.pyi:202:24: PYI052 Need type annotation for `CirculateRequest`
- stdlib/tkinter/__init__.pyi:203:21: PYI052 Need type annotation for `ClientMessage`
- stdlib/tkinter/__init__.pyi:204:16: PYI052 Need type annotation for `Colormap`
- stdlib/tkinter/__init__.pyi:205:17: PYI052 Need type annotation for `Configure`
- stdlib/tkinter/__init__.pyi:206:24: PYI052 Need type annotation for `ConfigureRequest`
- stdlib/tkinter/__init__.pyi:207:14: PYI052 Need type annotation for `Create`
... 28 additional changes omitted for project

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

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

+ rotkehlchen/chain/ethereum/modules/aave/v2/accountant.py:34:30: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ rotkehlchen/chain/ethereum/modules/aave/v2/accountant.py:71:30: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ rotkehlchen/chain/evm/decoding/cowswap/decoder.py:199:41: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ rotkehlchen/tasks/assets.py:441:29: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ rotkehlchen/tasks/manager.py:331:60: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ rotkehlchen/tasks/manager.py:341:39: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ rotkehlchen/tests/api/blockchain/test_evm.py:432:43: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ rotkehlchen/tests/unit/globaldb/test_upgrades.py:642:32: RUF031 [*] Avoid parentheses for tuples in subscripts.

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

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

+ analytics/lib/counts.py:540:28: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ zerver/actions/message_edit.py:961:17: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ zerver/lib/email_notifications.py:636:32: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ zerver/lib/email_notifications.py:638:32: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ zerver/lib/mention.py:133:33: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ zerver/lib/rate_limiter.py:222:33: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ zerver/lib/rate_limiter.py:243:30: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ zerver/lib/rate_limiter.py:264:13: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ zerver/lib/rate_limiter.py:266:42: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ zerver/lib/user_topics.py:290:36: RUF031 [*] Avoid parentheses for tuples in subscripts.
... 2 additional changes omitted for project

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

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

+ indico/modules/rb/models/reservations.py:82:32: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ indico/modules/users/util.py:312:30: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ indico/modules/users/util.py:326:34: RUF031 [*] Avoid parentheses for tuples in subscripts.
+ indico/modules/users/util.py:76:13: RUF031 [*] Avoid parentheses for tuples in subscripts.

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

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

- testing/test_config.py:1005:9: PLR6301 Method `test_inifilename` could be a function, class method, or static method
- testing/test_config.py:1089:13: PLR6301 Method `load` could be a function, class method, or static method
- testing/test_config.py:1124:13: PLR6301 Method `load` could be a function, class method, or static method
- testing/test_config.py:1151:13: PLR6301 Method `load` could be a function, class method, or static method
- testing/test_config.py:1179:13: PLR6301 Method `load` could be a function, class method, or static method
- testing/test_config.py:120:9: PLR6301 Method `test_ini_names` could be a function, class method, or static method
... 86 additional changes omitted for rule PLR6301
- testing/test_config.py:15:28: PLC2701 Private name import `_pytest`
- testing/test_config.py:16:28: PLC2701 Private name import `_pytest`
- testing/test_config.py:17:28: PLC2701 Private name import `_pytest`
- testing/test_config.py:18:28: PLC2701 Private name import `_pytest`
- testing/test_config.py:1978:43: PLC2701 Private name import `_pytest`
- testing/test_config.py:1978:5: PLC0415 `import` should be at the top-level of a file
- testing/test_config.py:19:28: PLC2701 Private name import `_pytest`
... 10 additional changes omitted for rule PLC2701
- testing/test_config.py:34:1: PLR0904 Too many public methods (23 > 20)
... 98 additional changes omitted for project

Changes by rule (37 rules affected)

code total + violation - violation + fix - fix
RUF031 126 126 0 0 0
PLR6301 92 0 92 0 0
PYI052 38 0 38 0 0
D212 19 0 19 0 0
PLC2701 15 0 15 0 0
D200 13 0 13 0 0
ANN401 12 0 12 0 0
DOC201 9 0 9 0 0
TRY003 9 0 9 0 0
EM102 6 0 6 0 0
DOC501 4 0 4 0 0
D401 3 0 3 0 0
EM101 3 0 3 0 0
PLC0415 3 0 3 0 0
TCH002 3 0 3 0 0
ARG002 2 0 2 0 0
COM812 2 0 2 0 0
E501 2 0 2 0 0
ARG004 2 0 2 0 0
PLR2004 2 0 2 0 0
D107 2 0 2 0 0
PLR0904 2 0 2 0 0
PLC1901 2 0 2 0 0
RUF022 1 0 1 0 0
D102 1 0 1 0 0
CPY001 1 0 1 0 0
RUF012 1 0 1 0 0
FBT001 1 0 1 0 0
FBT002 1 0 1 0 0
SIM102 1 0 1 0 0
ANN204 1 0 1 0 0
E721 1 0 1 0 0
B905 1 0 1 0 0
DOC402 1 0 1 0 0
ERA001 1 0 1 0 0
TCH003 1 0 1 0 0
UP035 1 0 1 0 0

@dylwil3 dylwil3 marked this pull request as ready for review August 3, 2024 04:19
@dylwil3
Copy link
Contributor Author

dylwil3 commented Aug 3, 2024

Any chance a reviewer could check whether the new lint.ruff option appears in "Settings" in the docs? For some reason I didn't see it and couldn't figure out why, but I may be messing up the local docs generation.

They appear in the json schema but not in the output of cargo dev generate-options.

If it matters, this is the first example of a "plugin" configuration being added to LintOptions rather than LintCommonOptions as this dosctring warning suggested - so hopefully I interpreted that correctly.

@dylwil3
Copy link
Contributor Author

dylwil3 commented Aug 3, 2024

Ok, should be good to go!

@dylwil3 dylwil3 changed the title [ruff] Implement parentheses-in-tuple-slices (RUF031) [ruff] Implement bad-format-tuple-in-getitem (RUF031) Aug 3, 2024
@MichaReiser
Copy link
Member

MichaReiser commented Aug 5, 2024

Hmm. I'm not sure I'd call this a "restriction" rule since it doesn't forbid the user from using any Python idioms that they'd otherwise be able to use.

It doesn't restrict the use of any python idoms but it does restrict the allowed syntax. Any use of subscript > tuple is restricted.

I'd call it a formatting rule, similar to many of our pycodestyle rules.

Fair enough

(similar to e.g. our flake8-pytest-style rules) -- so you can either enforce that there are never parentheses, or that there are always parentheses.

I'm sorry to raise this so late. I'm concerned about this option because it limits the formatter's style guide options in the long term. Our formatter already removes unnecessary parentheses in some places, and I could see it removing more parentheses in the long term. That's why I would prefer not support the option and merge the rule without.

@AlexWaygood
Copy link
Member

It doesn't restrict the use of any python idoms but it does restrict the allowed syntax. Any use of subscript > tuple is restricted.

I'm concerned that the implied definition of a "restriction" rule here is quite broad. Almost every rule that we have "restricts" you from doing something that the rule deems bad in terms of correctness, style or formatting :-) But this is getting off-topic.

I'm concerned about this option because it limits the formatter's style guide options in the long term. Our formatter already removes unnecessary parentheses in some places, and I could see it removing more parentheses in the long term. That's why I would prefer not support the option and merge the rule without.

Hmm, I see. To me it feels very unclear which style is preferable, which is why I think this makes sense as a configurable linter rule rather than something the formatter should have an opinion on. But if we think this is something the formatter might want to take an opinion on in the future, then I suppose I'm not sure that this is a linter rule we should have at all, then -- perhaps we should simply teach the formatter to have an opinion on this.

@MichaReiser
Copy link
Member

perhaps we should simply teach the formatter to have an opinion on this.

It doesn't seem that we feel comfortable making that decision today. You explicitly state that you are unclear about what style you prefer.

I wonder if the goal of the rule is to assert consistent formatting or if it is really about specifically restricting parenthesized tuples in subscriptions. What I hear is mainly that some people don't want to have parenthesized tuples in subscripts and less so that they want to enforce parentheses everywhere. Specifically restricting the use of parenthesized tuples also helps to give the rule a better name: allow(parenthesized-tuple-in-subscript) .

@AlexWaygood
Copy link
Member

perhaps we should simply teach the formatter to have an opinion on this.

It doesn't seem that we feel comfortable making that decision today. You explicitly state that you are unclear about what style you prefer.

Yep. That's why I don't think we should teach the formatter to have an opinion on this (ever), and why I would prefer a configurable linter rule ;-)

Anyway, I've said my piece on this. For me, I can see it's useful to enforce a consistent style across a codebase, but I think the rule is too opinionated -- as either something to be incorporated in the formatter or a linter rule -- unless it can be made configurable. I think we're going round in circles now, so I'll bow out at this point :-)

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

@AlexWaygood and I discussed this offline and I now agree that this isn't something the formatter should be opinionated about. Therefore, the lint rule as is is fine :)

I would prefer to change the name: bad isn't very telling. How about incorrectly-parenthesized-tuple-in-subscript or incorrectly-parenthesized-tuple-in-getitem?

For the setting, I think I would go with parenthesize-tuple-in-getitem to match the rule name.

@MichaReiser MichaReiser added accepted Ready for implementation and removed needs-decision Awaiting a decision from a maintainer labels Aug 5, 2024
@dylwil3
Copy link
Contributor Author

dylwil3 commented Aug 5, 2024

Great! I've made the requested changes. Let me know if there's anything else, and thanks for the discussion!

@dylwil3 dylwil3 changed the title [ruff] Implement bad-format-tuple-in-getitem (RUF031) [ruff] Implement incorrectly-parenthesized-tuple-in-getitem (RUF031) Aug 5, 2024
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for working on this. I left a few smaller nits and we may want to change the wording of the rule (@AlexWaygood )

// In case we want to change the boolean setting for
// this rule to an enum, this will make the code change
// just a little simpler.
#![allow(clippy::match_bool)]
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to use an if...else over a match instead of suppressing the clippy warning.

Copy link
Member

@AlexWaygood AlexWaygood Aug 7, 2024

Choose a reason for hiding this comment

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

Agreed. Sometimes in this kind of situation it can be more readable to introduce a custom enum and match on that instead, e.g.

enum ParenthesesPreference {
    Keep,
    Remove,
}

but here I'm not sure I really see the need; I think I'd just stick with bool

Copy link
Member

Choose a reason for hiding this comment

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

It looks like this has been marked as resolved, but the comment hasn't been addressed

@dylwil3 dylwil3 requested a review from carljm as a code owner August 7, 2024 11:26
@AlexWaygood
Copy link
Member

AlexWaygood commented Aug 7, 2024

Looks like you might have done a bad rebase there @dylwil3 -- the diff for this PR is now +7,002 −438 lines, which feels a bit overkill ;-)

@dylwil3 dylwil3 changed the title [ruff] Implement incorrectly-parenthesized-tuple-in-getitem (RUF031) [ruff] Implement incorrectly-parenthesized-tuple-in-subscript (RUF031) Aug 7, 2024
@AlexWaygood AlexWaygood removed the request for review from carljm August 7, 2024 11:49
@AlexWaygood
Copy link
Member

AlexWaygood commented Aug 7, 2024

@dylwil3, I fixed up your PR history for you. Before pushing any further updates to this PR, you'll now need to run the following commands locally in order to fetch your PR branch as it exists on GitHub (assuming your GitHub remote is called origin):

git fetch origin
git reset origin/getitem-with-parens-tuple --hard

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Nearly there!

// In case we want to change the boolean setting for
// this rule to an enum, this will make the code change
// just a little simpler.
#![allow(clippy::match_bool)]
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this has been marked as resolved, but the comment hasn't been addressed

@dylwil3
Copy link
Contributor Author

dylwil3 commented Aug 7, 2024

Thanks for your help cleaning up my mess 😄

Sorry for the last minute trouble and let me know if you find any other issues!

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM! I'll apply my final suggestions and then merge

@AlexWaygood AlexWaygood enabled auto-merge (squash) August 7, 2024 13:04
@AlexWaygood AlexWaygood merged commit 7997da4 into astral-sh:main Aug 7, 2024
18 checks passed
@dylwil3 dylwil3 deleted the getitem-with-parens-tuple branch August 7, 2024 13:30
@AlexWaygood
Copy link
Member

Thanks for the PR @dylwil3 and for your patience -- this was a great contribution!

@neutrinoceros
Copy link

Many thanks to y'all, I'm thrilled this is now a thing !

@dhruvmanila dhruvmanila added the preview Related to preview mode features label Aug 8, 2024
This pull request was closed.
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.

FR: format (or lint with fix) getitem called with tuples litteral (e.g. a[(b, c)] -> a[b, c]) ?
5 participants