Skip to content

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

Merged
MichaReiser merged 3 commits intoastral-sh:mainfrom
alex-700:latyshev/furb142
Mar 27, 2024
Merged

[refurb] Implement for-loop-set-mutations (FURB142)#10583
MichaReiser merged 3 commits intoastral-sh:mainfrom
alex-700:latyshev/furb142

Conversation

@alex-700
Copy link
Contributor

@alex-700 alex-700 commented Mar 25, 2024

Summary

Implement manual_set_update (FURB142) lint.
see:

Test Plan

cargo test

@github-actions
Copy link
Contributor

github-actions bot commented Mar 25, 2024 •
edited
Loading

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
MichaReiser
MichaReiser approved these changes Mar 26, 2024
View reviewed changes
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

AlexWaygood commented Mar 26, 2024

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".

AlexWaygood
AlexWaygood reviewed Mar 26, 2024
View reviewed changes
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 •
edited
Loading

@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!

AlexWaygood
AlexWaygood approved these changes Mar 26, 2024
View reviewed changes
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
@BrewTestBot BrewTestBot mentioned this pull request Apr 1, 2024
Merged
@alex-700 alex-700 deleted the latyshev/furb142 branch June 10, 2025 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MichaReiser MichaReiser MichaReiser approved these changes

@AlexWaygood AlexWaygood AlexWaygood approved these changes

Assignees

No one assigned

Labels

rule Implementing or modifying a lint rule

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@alex-700 @AlexWaygood @MichaReiser