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

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

Merged
merged 13 commits into from
Dec 1, 2023

Conversation

diceroll123
Copy link
Contributor

@diceroll123 diceroll123 commented Oct 18, 2023

Summary

Add R1733 and autofix!

See #970

Test Plan

cargo test and manually

@diceroll123
Copy link
Contributor Author

This PR is practically just #7999 in a trenchcoat

@github-actions

This comment was marked as outdated.

@diceroll123 diceroll123 changed the title [pylint] - add unnecessary_dict_index_lookup (R1733) + autofix [pylint] - add unnecessary_dict_index_lookup (PLR1733) + autofix Oct 18, 2023
@Skylion007
Copy link
Contributor

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

@diceroll123
Copy link
Contributor Author

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!

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

@charliermarsh Anything blocking these two PRs btw?

@charliermarsh
Copy link
Member

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

@Skylion007
Copy link
Contributor

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link

codspeed-hq bot commented Nov 4, 2023

CodSpeed Performance Report

Merging #8036 will not alter performance

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

Summary

✅ 30 untouched benchmarks

Copy link
Contributor

github-actions bot commented Nov 4, 2023

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

@@ -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),
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rebased and fixed!

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

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

@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
Copy link
Contributor Author

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

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

Thanks!

@charliermarsh charliermarsh changed the title [pylint] - add unnecessary_dict_index_lookup (PLR1733) + autofix [pylint] Implement unnecessary-dict-index-lookup (PLR1733) Dec 1, 2023
@charliermarsh charliermarsh added rule Implementing or modifying a lint rule preview Related to preview mode features labels Dec 1, 2023
@charliermarsh charliermarsh enabled auto-merge (squash) December 1, 2023 05:04
@diceroll123
Copy link
Contributor Author

Thank YOU! 😄

@charliermarsh charliermarsh merged commit cb1d3df into astral-sh:main Dec 1, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants