-
Notifications
You must be signed in to change notification settings - Fork 749
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
Make tested code valid Python code #657
base: main
Are you sure you want to change the base?
Conversation
Update fork
Playing with AST checks can be very tedious because of the amount of "incorrect" Python code which raises E901 (or fail differently) when AST checks are enabled. An easy solution is to fix the code but this is not always applicable. I've only performed the abundant and trivial changes.
@@ -871,17 +871,15 @@ def whitespace_around_named_parameter_equals(logical_line, tokens): | |||
Don't use spaces around the '=' sign when used to indicate a | |||
keyword argument or a default parameter value. | |||
|
|||
Okay: def complex(real, imag=0.0): | |||
Okay: return magic(r=real, i=imag) | |||
Okay: def complex(real, imag=0.0):\n return magic(r=real, i=imag) |
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.
These were two separate checks for two separate test cases. Collapsing them may introduce bugs
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.
The whole thing is still tested, isn't it ? Or I can write:
def complex(real, imag=0.0): pass
and
magic(r=real, i=imag)
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 would rather retain two separate tests cases.
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 am suggesting 2 separate test cases, each of them corresponding to a piece of valid Python code.
@@ -1,13 +1,15 @@ | |||
#: E111 | |||
if x > 2: | |||
print x | |||
print(x) |
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.
This means we'll loose coverage for the print statement then, yes?
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 am not sure I fully understand what you mean. The E11 errors does not seem to be related to print and/or parentheses in any way.
@@ -1,22 +1,22 @@ | |||
#: E121 | |||
print "E121", ( |
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.
Same concern as above
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.
If parentheses are important for these tests (which is likely), maybe I could replace the
print expression
with
raise expression
or
assert expression
Playing with AST checks can be very tedious
because of the amount of "incorrect" Python
code which raises E901 (or fail differently)
when AST checks are enabled.
An easy solution is to fix the code but this
is not always applicable.
I've only performed the trivial changes.