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

Make tested code valid Python code #657

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

SylvainDe
Copy link

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.

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)
Copy link
Member

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

Copy link
Author

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)

Copy link
Member

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.

Copy link
Author

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)
Copy link
Member

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?

Copy link
Author

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", (
Copy link
Member

Choose a reason for hiding this comment

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

Same concern as above

Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants