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

Ouput bad comment during syntax error #6779

Merged
merged 9 commits into from May 6, 2019

Conversation

@kcaebe
Copy link
Contributor

commented May 6, 2019

fixes #4103

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

commented May 6, 2019

You need to update existing tests. They are currently failing.

@gvanrossum

This comment has been minimized.

Copy link
Member

commented May 6, 2019

This is a good idea, but I'd like to see some tests (and the existing tests that are failing fixed). You also ought to add the same thing to fastparse2.py.

@@ -192,7 +192,8 @@ def parse_type_comment(type_comment: str,
typ = ast3_parse(type_comment, '<type_comment>', 'eval')
except SyntaxError as e:
if errors is not None:
errors.report(line, e.offset, TYPE_COMMENT_SYNTAX_ERROR, blocker=True)
err_msg = "{} '{}'".format(TYPE_COMMENT_SYNTAX_ERROR, type_comment)

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 6, 2019

Collaborator

I would strip everything in the type_comment after #, so that only the interesting part is shown.

file:2: error: syntax error in type comment
file:2: error: syntax error in type comment '*'

[case testInvalidTypeComment5

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 6, 2019

Collaborator

You have missing closing ] here in in couple tests below.

kcaebe added some commits May 6, 2019

@ilevkivskyi
Copy link
Collaborator

left a comment

LGTM now! I will merge when tests pass in CI.

@@ -337,6 +337,7 @@ def visit_FunctionDef(self, n: ast27.FunctionDef) -> Statement:
return_type = None
elif type_comment is not None and len(type_comment) > 0:
try:
print(type_comment)

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 6, 2019

Collaborator

Stray debugging print left.

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

commented May 6, 2019

Your last commit will likely cause many failures in CI (because at least some existing tests need to be updated). It is much easier to run tests locally first, since now the line in CI is really long, your PR might wait for an hour there.

@kcaebe

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

I wanted to get the function annotations in the same PR so I'm okay with it taking a bit. Thanks for all the help @ilevkivskyi.

@ilevkivskyi ilevkivskyi merged commit 2c6cd83 into python:master May 6, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.