-
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
include tuples in b018 useless-statement check #432
Conversation
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.
Read again this morning and still confused at some of the test results ... I feel I'm missing something - Can you explain my missing context please :)
@@ -270,6 +270,8 @@ def test_b018_functions(self): | |||
expected = [B018(line, 4) for line in range(15, 25)] | |||
expected.append(B018(28, 4)) | |||
expected.append(B018(31, 4)) | |||
expected.append(B018(32, 4)) | |||
expected.append(B018(33, 4)) |
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.
Why do we not need 34 here when we include 32 for modules tests? What am I missing here, cause the variable assign happens in a function it's fine?
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.
Oh it's a little confusing because the line numbers for the three different test cases are referring to the 3 different b018_....py
files (but the line numbers are almost the same for 2 of the files). Added the same 3 lines to each of the files and corresponding test cases, in all cases the first 2 lines are bad and the 3rd is good
(1,) # bad
(2, 3) # bad
t = (4, 5) # good
Something else to note here - a dangling comma after a single thing (e.g. a function call) will turn it into a tuple, which would get flagged as a useless statement: print("abc") # this is fine print("abc"), # the comma essentially makes this a tuple (None,) so this would be flagged as a useless expression. ^ wondering if this might be confusing and should be labelled a little differently? Or even as separate error? E.g. |
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.
Ok, I get it now. Yeah, I like maybe adding the "Do you have a trailing comma" at least in the README should help some lost people ...
I was looking for something to detect useless tuple statements, noticed this had a useless-statement check but didn't include tuples. Any reason for that? Otherwise, figured we could add it?