Generalize PIE807 to handle dict literals by alanhdu · Pull Request #8608 · astral-sh/ruff

alanhdu · 2023-11-10T19:42:41Z

Summary

PIE807 will rewrite lambda: [] to list -- AFAICT though, the same rationale also applies to dicts, so I've modified the code to also rewrite lambda: {} to dict.

Two things I'm not sure about:

Should this go to a new rule? This no longer actually matches the behavior of flake8-pie, and while I think thematically it makes sense to be part of the same rule, we could make it a standalone rule (but if so, where should I put it and what error code should I use)?
If we want a single rule, are there backwards compatibility concerns with the rule name change (from reimplemented_list_builtin to reimplemented_container_builtin?

Test Plan

Added snapshot tests of the functionality.

github-actions · 2023-11-10T20:00:09Z

`ruff-ecosystem` results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+1 -0 violations, +0 -0 fixes in 41 projects)

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

+ rotkehlchen/tests/api/test_balances.py:953:135: PIE807 [*] Prefer `dict` over useless lambda

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
PIE807	1	1	0	0	0

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+6 -0 violations, +0 -0 fixes in 41 projects)

demisto/content (+5 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ Packs/RubrikPolaris/Integrations/RubrikPolaris/RubrikPolaris_test.py:61:45: PIE807 [*] Prefer `dict` over useless lambda
+ Packs/VMware/Integrations/VMware/VMware_test.py:593:48: PIE807 [*] Prefer `dict` over useless lambda
+ Packs/VMware/Integrations/VMware/VMware_test.py:649:48: PIE807 [*] Prefer `dict` over useless lambda
+ Packs/VMware/Integrations/VMware/VMware_test.py:677:48: PIE807 [*] Prefer `dict` over useless lambda
+ Packs/VMware/Integrations/VMware/VMware_test.py:721:48: PIE807 [*] Prefer `dict` over useless lambda

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

ruff check --no-cache --exit-zero --preview

+ rotkehlchen/tests/api/test_balances.py:953:135: PIE807 [*] Prefer `dict` over useless lambda

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
PIE807	6	6	0	0	0

charliermarsh · 2023-11-10T20:51:10Z

👍 I think it's fine to use a single rule, and even to rename it (I believe our versioning policy allows this), but we should probably gate the change in behavior to preview-only.

charliermarsh · 2023-11-10T20:51:35Z

You can grep for fn preview_rules for an example of testing preview rules, and to find rules that currently deviate based on the preview flag.

zanieb · 2023-11-10T21:01:39Z

crates/ruff_linter/src/rules/flake8_pie/rules/reimplemented_container_builtin.rs

-pub struct ReimplementedListBuiltin;
+pub struct ReimplementedContainerBuiltin;


Maybe just ReimplementedBuiltin would suffice?

I think we have some other rules that also detect re-implemented builtins though -- like the simplify rules that detect re-implemented any and all loops.

Hm yes ReimplementedBuiltin is used for SIM110 and SIM111 — that's such a generic name for those though...

I think ReimplementedContainerBuiltin is fine but separately we should consider changing that other one to be more specific :D

zanieb · 2023-11-10T21:03:39Z

crates/ruff_linter/src/rules/flake8_pie/rules/reimplemented_container_builtin.rs

    fn message(&self) -> String {
-        format!("Prefer `list` over useless lambda")
+        format!("Prefer `list` or `dict` over useless lambda")
    }


You could store the type on the Violation then say "Prefer list over lambda" or "Prefer dict over lambda" depending on the case. It seems confusing to present both to the user when we know which they meant.

Agreed! Can grep for DeferralKeyword as an example of this pattern.

Skylion007 · 2023-11-11T14:59:08Z

I remember reviewing a PR for a more general rule about any lambda that didn't pass any args to a function call. Would that be a better rule to enable? I think it was a pylint one.

charliermarsh · 2023-11-11T15:04:03Z

I think it's #7953, although I think that rule may not catch this case because it's not aware that (e.g.) [] is equivalent to list.

Skylion007 · 2023-11-11T15:25:06Z

Ah, you are right. PIE807 doesn't actually even catch

doc_string_dict = defaultdict(lambda: list())

but unnecessary lambda does.

alanhdu · 2023-11-13T15:50:10Z

crates/ruff_linter/src/rules/flake8_pie/rules/reimplemented_container_builtin.rs

+///
+/// [preview]: https://docs.astral.sh/ruff/preview/
+#[violation]
+pub struct ReimplementedContainerBuiltin(&'static str);


I could make this an enum instead, but not sure it's worthwhile compared to just storing the builtin inline.

I decided to make it an enum to follow the pattern we use elsewhere, even though it's really unimportant.

charliermarsh

Thanks!

charliermarsh · 2023-11-13T17:46:58Z

(The Rotki ecosystem change is expected as they use preview mode by default.)

alanhdu added 2 commits November 10, 2023 14:31

Generalize PIE807 to include dict literals

56eb170

Update snpashot tests

d4af4f5

charliermarsh self-requested a review November 10, 2023 20:51

charliermarsh added the rule Implementing or modifying a lint rule label Nov 10, 2023

zanieb reviewed Nov 10, 2023

View reviewed changes

zanieb mentioned this pull request Nov 10, 2023

Add check for is comparison with mutable initialisers to rule F632 #8607

Merged

Make PIE807 dict fix preview only

b630dde

alanhdu force-pushed the alan/container-builtins branch from a3ba70f to b630dde Compare November 13, 2023 15:45

alanhdu commented Nov 13, 2023

View reviewed changes

charliermarsh added 2 commits November 13, 2023 12:45

Use an enum

1154cbf

Merge branch 'main' into alan/container-builtins

05930bc

charliermarsh approved these changes Nov 13, 2023

View reviewed changes

Tweak docs

0ef31d4

charliermarsh enabled auto-merge (squash) November 13, 2023 17:46

Clippy

e14024e

charliermarsh merged commit 6f23bdb into astral-sh:main Nov 13, 2023

alanhdu deleted the alan/container-builtins branch November 16, 2023 02:33

miccal mentioned this pull request Nov 17, 2023

ruff 0.1.6 Homebrew/homebrew-core#154716

Merged

		pub struct ReimplementedListBuiltin;
		pub struct ReimplementedContainerBuiltin;

Conversation

alanhdu commented Nov 10, 2023

Summary

Test Plan

Uh oh!

github-actions bot commented Nov 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

charliermarsh commented Nov 10, 2023

Uh oh!

charliermarsh commented Nov 10, 2023

Uh oh!

zanieb Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

charliermarsh Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

zanieb Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

zanieb Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

charliermarsh Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

Skylion007 commented Nov 11, 2023

Uh oh!

charliermarsh commented Nov 11, 2023

Uh oh!

Skylion007 commented Nov 11, 2023

Uh oh!

alanhdu Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

charliermarsh Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

charliermarsh left a comment

Choose a reason for hiding this comment

Uh oh!

charliermarsh commented Nov 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented Nov 10, 2023 •

edited

Loading

`ruff-ecosystem` results