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

[refurb] Implement for-loop-set-mutations (FURB142) #10583

Merged
merged 3 commits into from
Mar 27, 2024

Conversation

alex-700
Copy link
Contributor

Summary

Implement manual_set_update (FURB142) lint.
see:

Test Plan

cargo test

Copy link
Contributor

github-actions bot commented Mar 25, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

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

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

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

+ airflow/providers/amazon/aws/hooks/glue_catalog.py:124:13: FURB142 [*] Use of `set.add()` in a for loop
+ airflow/providers/amazon/aws/hooks/glue_catalog.py:82:13: FURB142 [*] Use of `set.add()` in a for loop
+ airflow/utils/dag_edges.py:74:17: FURB142 [*] Use of `set.add()` in a for loop
+ dev/breeze/src/airflow_breeze/commands/release_management_commands.py:2052:13: FURB142 [*] Use of `set.add()` in a for loop
+ dev/breeze/src/airflow_breeze/utils/docker_command_utils.py:522:5: FURB142 [*] Use of `set.add()` in a for loop
+ dev/breeze/src/airflow_breeze/utils/provider_dependencies.py:54:9: FURB142 [*] Use of `set.add()` in a for loop
+ dev/breeze/src/airflow_breeze/utils/selective_checks.py:827:17: FURB142 [*] Use of `set.add()` in a for loop
+ dev/stats/calculate_statistics_provider_testing_issues.py:110:5: FURB142 [*] Use of `set.add()` in a for loop
+ dev/stats/calculate_statistics_provider_testing_issues.py:117:5: FURB142 [*] Use of `set.add()` in a for loop
+ scripts/tools/list-integrations.py:67:9: FURB142 [*] Use of `set.add()` in a for loop
+ tests/jobs/test_scheduler_job.py:3143:9: FURB142 [*] Use of `set.add()` in a for loop
+ tests/jobs/test_scheduler_job.py:3157:9: FURB142 [*] Use of `set.add()` in a for loop

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

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

+ tests/unit/bokeh/document/test_models.py:113:9: FURB142 [*] Use of `set.add()` in a for loop

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

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

+ rotkehlchen/accounting/structures/defi.py:94:13: FURB142 [*] Use of `set.add()` in a for loop
+ rotkehlchen/chain/bitcoin/bch/utils.py:166:5: FURB142 [*] Use of `set.add()` in a for loop
+ rotkehlchen/chain/evm/decoding/velodrome/velodrome_cache.py:118:9: FURB142 [*] Use of `set.add()` in a for loop
+ rotkehlchen/chain/evm/decoding/velodrome/velodrome_cache.py:122:9: FURB142 [*] Use of `set.add()` in a for loop
+ rotkehlchen/tests/exchanges/test_okx.py:33:5: FURB142 [*] Use of `set.add()` in a for loop
+ rotkehlchen/tests/unit/test_data_dir.py:77:5: FURB142 [*] Use of `set.add()` in a for loop

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

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

+ corporate/views/support.py:501:9: FURB142 [*] Use of `set.add()` in a for loop
+ corporate/views/support.py:506:9: FURB142 [*] Use of `set.add()` in a for loop
+ corporate/views/support.py:515:9: FURB142 [*] Use of `set.add()` in a for loop
+ corporate/views/support.py:526:9: FURB142 [*] Use of `set.add()` in a for loop
+ zerver/data_import/mattermost.py:221:9: FURB142 [*] Use of `set.add()` in a for loop
+ zerver/data_import/mattermost.py:224:9: FURB142 [*] Use of `set.add()` in a for loop
+ zerver/data_import/mattermost.py:260:13: FURB142 [*] Use of `set.add()` in a for loop
+ zerver/data_import/rocketchat.py:256:9: FURB142 [*] Use of `set.add()` in a for loop
+ zerver/lib/bulk_create.py:250:5: FURB142 [*] Use of `set.add()` in a for loop
+ zerver/lib/display_recipient.py:157:5: FURB142 [*] Use of `set.add()` in a for loop
+ zerver/lib/server_initialization.py:84:5: FURB142 [*] Use of `set.add()` in a for loop
+ zerver/lib/test_fixtures.py:357:17: FURB142 [*] Use of `set.add()` in a for loop
+ zerver/tests/test_import_export.py:1126:21: FURB142 [*] Use of `set.add()` in a for loop
+ zerver/views/message_send.py:43:9: FURB142 [*] Use of `set.add()` in a for loop

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

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

+ indico/modules/events/abstracts/controllers/reviewing.py:199:13: FURB142 [*] Use of `set.add()` in a for loop

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
FURB142 34 34 0 0 0

@MichaReiser MichaReiser added the rule Implementing or modifying a lint rule label Mar 26, 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.

Thanks. This looks good to me. But I want to leave it to @AlexWaygood to also take a look.

My only concern with the rule as is is its name. From reading the rule name (or even the error message), it gives me the impression that I'm not supposed to update a set "manually" (unclear what that means) at all. Maybe for-set-mutations or for-iterable-set-mutations

My reasoning. This is a style rule that guides users towards writing more idiomatic Python.

@AlexWaygood
Copy link
Member

My only concern with the rule as is is its name. From reading the rule name (or even the error message), it gives me the impression that I'm not supposed to update a set "manually" (unclear what that means) at all. Maybe for-set-mutations or for-iterable-set-mutations

I agree with @MichaReiser. The original name that Refurb has seems quite good to me (no-set-for-loop), but obviously doesn't fit with our naming conventions. Maybe for-loop-set-mutations? In the error message, rather than saying "use of manual set update", I'd probably just say "use of set.add in a for loop".

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.

Overall really nicely done -- seems like a useful, uncontroversial rule and the autofixes are great. Just need to work on the docs a little bit :-)

@AlexWaygood AlexWaygood changed the title [refurb] Implement manual_set_update (FURB142) [refurb] Implement for-loop-set-mutations (FURB142) Mar 26, 2024
@alex-700
Copy link
Contributor Author

alex-700 commented Mar 26, 2024

@MichaReiser @AlexWaygood thanks for the review!

My only concern with the rule as is is its name. From reading the rule name (or even the error message), it gives me the impression that I'm not supposed to update a set "manually" (unclear what that means) at all. Maybe for-set-mutations or for-iterable-set-mutations

I'm fully aggree with the naming concern here, but could not invent something better.
Stick to @AlexWaygood 's option for-loop-set-mutations, which I really like. Thanks!

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, thanks!

@AlexWaygood AlexWaygood requested a review from MichaReiser March 26, 2024 11:38
@MichaReiser MichaReiser merged commit f9d0c6d into astral-sh:main Mar 27, 2024
17 checks passed
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.

3 participants