[`pylint`] Implement `unnecessary-dict-index-lookup` (`PLR1733`) #8036

diceroll123 · 2023-10-18T04:11:56Z

Summary

Add R1733 and autofix!

See #970

Test Plan

cargo test and manually

diceroll123 · 2023-10-18T04:17:15Z

This PR is practically just #7999 in a trenchcoat

Skylion007 · 2023-10-19T16:59:02Z

Does this pylint rule cover dict comprehensions? or would that be covered by another rule? Or is it just not covered sadly?

diceroll123 · 2023-10-19T17:40:23Z

Does this pylint rule cover dict comprehensions? or would that be covered by another rule? Or is it just not covered sadly?

ooh, that was an oversight, I'll add all comprehensions later, thanks!

Skylion007 · 2023-10-29T18:39:49Z

@charliermarsh Anything blocking these two PRs btw?

charliermarsh · 2023-10-29T18:42:00Z

@Skylion007 - Just me finding time to review it, there's a lot of logic so I just need to read it closely.

Skylion007 · 2023-11-04T17:37:30Z

I would add a test case for this bug: #7999 (comment) . I suspect you would hit it with dict comprehensions as well.

Skylion007 · 2023-11-04T17:38:24Z

crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_dict_index_lookup.py

+        FRUITS[fruit_name]: int = FRUITS[fruit_name]  # Ok
+        FRUITS[fruit_name] = FRUITS[fruit_name]  # Ok
+        FRUITS[fruit_name] += FRUITS[fruit_name]  # Ok
+        FRUITS[fruit_name] = "d"  # Ok


What about accessing _ after FRUITS[fruit_name] is edited? that's the main thing missing here.

codspeed-hq · 2023-11-04T18:42:37Z

CodSpeed Performance Report

Merging #8036 will not alter performance

_{Comparing diceroll123:add-R1733 (92dc833) with main (69dfe0a)}

Summary

✅ 30 untouched benchmarks

github-actions · 2023-11-04T18:43:27Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

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

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

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

+ airflow/providers/amazon/aws/executors/ecs/utils.py:265:31: PLR1733 [*] Unnecessary lookup of dictionary value by key
+ airflow/providers/amazon/aws/hooks/glue.py:378:88: PLR1733 [*] Unnecessary lookup of dictionary value by key

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

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

+ rotkehlchen/tests/utils/checks.py:67:53: PLR1733 [*] Unnecessary lookup of dictionary value by key

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
PLR1733	3	3	0	0	0

Skylion007 · 2023-11-24T20:08:07Z

crates/ruff_linter/src/codes.rs

@@ -254,6 +254,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
        (Pylint, "R1711") => (RuleGroup::Stable, rules::pylint::rules::UselessReturn),
        (Pylint, "R1714") => (RuleGroup::Stable, rules::pylint::rules::RepeatedEqualityComparison),
        (Pylint, "R1706") => (RuleGroup::Preview, rules::pylint::rules::AndOrTernary),
+        (Pylint, "R1733") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryDictIndexLookup),


Shouldn't this be 1 row down to maintain sorted order?

rebased and fixed!

charliermarsh

@diceroll123 -- Thank you! Do you mind taking a look at some of the tweaks I made in #8932 and the comments I left to explain them, and applying those changes here?

diceroll123 · 2023-12-01T00:43:09Z

Will do, I see you're still adding comments so I'll check back later!

charliermarsh

Thanks!

diceroll123 · 2023-12-01T05:05:16Z

Thank YOU! 😄

This comment was marked as outdated.

Sign in to view

diceroll123 changed the title ~~[pylint] - add unnecessary_dict_index_lookup (R1733) + autofix~~ [pylint] - add unnecessary_dict_index_lookup (PLR1733) + autofix Oct 18, 2023

diceroll123 force-pushed the add-R1733 branch 4 times, most recently from 0b2c21d to a0cd6b1 Compare October 22, 2023 22:12

Skylion007 reviewed Nov 4, 2023

View reviewed changes

Skylion007 reviewed Nov 24, 2023

View reviewed changes

diceroll123 force-pushed the add-R1733 branch 2 times, most recently from 29e0e45 to e9dfe6b Compare November 30, 2023 23:52

diceroll123 added 10 commits November 30, 2023 18:58

add R1733 and autofix

98d91ac

add the other assignments

7f0213c

add generator/compehrension expressions

0307325

tweak logic for assignments

ae0806e

fix lint code order

d5c72f0

update snapshots

877382c

tweak wordings

4811f72

fix comments

9ae5ed2

fix snapshot

b4b47fb

tweaks

a6a286e

diceroll123 force-pushed the add-R1733 branch from e9dfe6b to a6a286e Compare November 30, 2023 23:59

charliermarsh reviewed Dec 1, 2023

View reviewed changes

Make suggested changes

b32cc39

diceroll123 requested a review from charliermarsh December 1, 2023 04:42

Tweaks

dac65ec

charliermarsh approved these changes Dec 1, 2023

View reviewed changes

charliermarsh changed the title ~~[pylint] - add unnecessary_dict_index_lookup (PLR1733) + autofix~~ [pylint] Implement unnecessary-dict-index-lookup (PLR1733) Dec 1, 2023

charliermarsh added rule Implementing or modifying a lint rule preview Related to preview mode features labels Dec 1, 2023

Merge branch 'main' into add-R1733

92dc833

charliermarsh enabled auto-merge (squash) December 1, 2023 05:04

charliermarsh merged commit cb1d3df into astral-sh:main Dec 1, 2023
16 checks passed

miccal mentioned this pull request Dec 5, 2023

ruff 0.1.7 Homebrew/homebrew-core#156391

Merged

trag1c mentioned this pull request Jan 11, 2024

Add PR title guidelines to CONTRIBUTING.md #9470

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[`pylint`] Implement `unnecessary-dict-index-lookup` (`PLR1733`) #8036

[`pylint`] Implement `unnecessary-dict-index-lookup` (`PLR1733`) #8036

diceroll123 commented Oct 18, 2023 •

edited

diceroll123 commented Oct 18, 2023

This comment was marked as outdated.

Skylion007 commented Oct 19, 2023

diceroll123 commented Oct 19, 2023

Skylion007 commented Oct 29, 2023

charliermarsh commented Oct 29, 2023

Skylion007 commented Nov 4, 2023

Skylion007 Nov 4, 2023

codspeed-hq bot commented Nov 4, 2023 •

edited

github-actions bot commented Nov 4, 2023 •

edited

Skylion007 Nov 24, 2023

diceroll123 Nov 24, 2023

charliermarsh left a comment

diceroll123 commented Dec 1, 2023

charliermarsh left a comment

diceroll123 commented Dec 1, 2023

[pylint] Implement unnecessary-dict-index-lookup (PLR1733) #8036

[pylint] Implement unnecessary-dict-index-lookup (PLR1733) #8036

Conversation

diceroll123 commented Oct 18, 2023 • edited

Summary

Test Plan

diceroll123 commented Oct 18, 2023

This comment was marked as outdated.

Skylion007 commented Oct 19, 2023

diceroll123 commented Oct 19, 2023

Skylion007 commented Oct 29, 2023

charliermarsh commented Oct 29, 2023

Skylion007 commented Nov 4, 2023

Skylion007 Nov 4, 2023

Choose a reason for hiding this comment

codspeed-hq bot commented Nov 4, 2023 • edited

CodSpeed Performance Report

Merging #8036 will not alter performance

Summary

github-actions bot commented Nov 4, 2023 • edited

ruff-ecosystem results

Linter (stable)

Linter (preview)

Skylion007 Nov 24, 2023

Choose a reason for hiding this comment

diceroll123 Nov 24, 2023

Choose a reason for hiding this comment

charliermarsh left a comment

Choose a reason for hiding this comment

diceroll123 commented Dec 1, 2023

charliermarsh left a comment

Choose a reason for hiding this comment

diceroll123 commented Dec 1, 2023

[`pylint`] Implement `unnecessary-dict-index-lookup` (`PLR1733`) #8036

[`pylint`] Implement `unnecessary-dict-index-lookup` (`PLR1733`) #8036

diceroll123 commented Oct 18, 2023 •

edited

codspeed-hq bot commented Nov 4, 2023 •

edited

github-actions bot commented Nov 4, 2023 •

edited

`ruff-ecosystem` results