-
Notifications
You must be signed in to change notification settings - Fork 109
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
Add B033: Duplicate items in sets #373
Conversation
Love it when I find a simpler solution within seconds of pushing. I'll blame it on me only using Python regularly for a few months! |
@@ -518,6 +518,10 @@ def visit_Import(self, node): | |||
self.check_for_b005(node) | |||
self.generic_visit(node) | |||
|
|||
def visit_Set(self, node): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do this for dict keys too? It should be fairly easy and it's also a reasonably common bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pyflakes (which is included in flake8) has a rule to detect duplicate keys with different values, F601 dictionary key name repeated with different values.
It doesn't flag duplicate keys with the same values, but as that's unlikely to cause a bug per se, should we check for that?
Edit: Although I guess you could say the same about duplicate entries in sets. If we wanted to add it it's fairly simple and I have a demo working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the extra context! Yes I think dicts with both keys and values the same are pretty much equivalent to duplicate set members. Honestly I don't have a great sense of what rules belong where, but maybe to avoid duplication we should only alert in cases where pyflakes doesn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, we would only want to flag when both the dictionary keys and values are the same to avoid conflicts with Pyflakes.
I think a new rule would be good for this (in case people want to disable each rule separately) and we could leave B033 as it is? As we'd be checking both the keys and the values it also wouldn't easily fit into this rule anyway, so I don't think we'd gain much by combining them into a single rule.
test = {1, True} | ||
test = {0, False} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do True and False booleans hash to the same as 0 and !0 ... TIL if so. Guess I've never thought about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, I also discovered this when creating the check.
For example, this:
test = {1, True}
print(test)
Will print:
{1}
This checks for duplicate items in sets when using the
{}
syntax.Adds a part of #371.