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

PLC0206 should not flag if dict is being written #14585

Open
silverwind opened this issue Nov 25, 2024 · 5 comments
Open

PLC0206 should not flag if dict is being written #14585

silverwind opened this issue Nov 25, 2024 · 5 comments
Labels
rule Implementing or modifying a lint rule

Comments

@silverwind
Copy link

This code is flagged for PLC0206 with v0.8.0:

foo = {}
for key in foo.keys(): # PLC0206 Extracting value from dictionary without calling `.items()`
  if foo[key]:
    foo[key] = True

Arguable, changing .keys() to .items() here does not improve readability because one still needs to use foo[key] to write into the dict:

foo = {}
for key, value in foo.items():
  if value:
    foo[key] = True

Suggestion would be to not trigger the rule when any write to the dict is present. Interestingly, the rule already does not flag when the if is removed.

@brodycj
Copy link

brodycj commented Nov 25, 2024

What about using .filter() to only update items that match your criteria?

@silverwind
Copy link
Author

What about using .filter() to only update items that match your criteria?

The code isn't really meant to be taken seriously and the actual code where I hit this was more complex. If you like a slightly more complex example, take this one, which is also flagged:

foo = {}
for key in foo.keys():
  if foo[key] == "foo":
    foo[key] = "bar"
  elif foo[key] == "bar":
    foo[key] = "foo"

@MichaReiser
Copy link
Member

That makes sense. I first thought that this is the same as #13821 but it's not because #13821 also reads the value while iterating

@MichaReiser MichaReiser added the rule Implementing or modifying a lint rule label Nov 25, 2024
@dylwil3
Copy link
Collaborator

dylwil3 commented Nov 26, 2024

Interestingly, the rule already does not flag when the if is removed.

That's because the rule triggers the first time it sees foo[key] in a load context - like in the if statement.

I'm actually kind of borderline on whether we should or shouldn't flag, especially because we already don't flag if foo[key] is only used when someone assigns a new value to it (or deletes it).

Already in your second example, it starts to get a little verbose. I kind of find it more readable:

for key,val in foo.items():
  if val == "foo":
    foo[key] = "bar"
  elif val == "bar":
    foo[key] = "foo"

One could imagine even more gymnastics:

for key in foo:
   # <--- all sorts of complicated expressions with the value foo[key] --->
   if cond_now_holds:
       foo[key] = 10

It seems in the spirit of the rule to flag this.

But I also see your point. I'm on the fence.

(P.S. - ignore the linked PR below. Bad copy pasting ... too many tabs open 😅 )

@MichaReiser
Copy link
Member

MichaReiser commented Nov 26, 2024

Oh, @dylwil3, you're right. This is the same as #13821. That's what you get for replying late in the day 😆

I then agree that the rule should flag this pattern because it helps remove some repetition. It's unfortunate that Python doesn't allow assigning to *value = new_value the same way as rust does

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

No branches or pull requests

4 participants