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

Ignore SyntaxError while parsing #214

Merged
merged 2 commits into from Oct 16, 2016

Conversation

Projects
3 participants
@c-w
Copy link
Contributor

c-w commented Oct 11, 2016

This is a partial fix for #168. With this change, the parser no longer crashes on test file E90.py.

Note that this is a pretty minimal fix. A potential future refactor to increase the robustness of the parser could be to use the null-object pattern and return a NullToken from TokenStream.move instead of None. In this way, any code that consumes TokenStream.current doesn't have to null-check the property value explicitly which should make the other issues mentioned in #168 go away.

@sigmavirus24

This comment has been minimized.

Copy link
Member

sigmavirus24 commented Oct 11, 2016

Could you add tests for this, please?

@c-w

This comment has been minimized.

Copy link
Contributor

c-w commented Oct 13, 2016

@sigmavirus24: Sure thing. 9615c72 adds a test for the fix in dc34871. This also required making a fix in the test harness: 839b528.

try:
return next(self._generator, None)
except SyntaxError as syntax_error:
self.log.warning('error generating tokens: {0}', syntax_error)

This comment has been minimized.

@Nurdok

Nurdok Oct 13, 2016

Member

Instead of capturing the exception and passing it, you can do:

self.log.warning('error generating tokens', exc_info=True)

This comment has been minimized.

@c-w

c-w Oct 14, 2016

Contributor

Good shout.

toto = 0 + 0
#:
""" # noqa

This comment has been minimized.

@Nurdok

Nurdok Oct 13, 2016

Member

Why do you need this huge snippet? You can create a syntax error with a single character (like "[").

This comment has been minimized.

@c-w

c-w Oct 14, 2016

Contributor

This snippet is the test file that was reported to cause issues in #168, but yes, we can minimize the test example. Turns out that the single invalid token (e.g. "[" like you mentioned) leads to a different exception: tk.TokenError so I added that as a test case too.

@c-w

This comment has been minimized.

Copy link
Contributor

c-w commented Oct 15, 2016

@Nurdok: Addressed comments and resolved merge conflicts.

pass
"""

source_token_error = """

This comment has been minimized.

@Nurdok

Nurdok Oct 15, 2016

Member

Why do you need this to be a multi-line string? Why not just source_token_error = "["?

This comment has been minimized.

@c-w

c-w Oct 15, 2016

Contributor

Done.

def _next_from_generator(self):
try:
return next(self._generator, None)
except (SyntaxError, tk.TokenError):

This comment has been minimized.

@Nurdok

Nurdok Oct 15, 2016

Member

Please also add this bugfix to the release notes.

This comment has been minimized.

@c-w

c-w Oct 15, 2016

Contributor

Added a line to docs/release_notes.rst.

Fix parser test case for invalid source
Commit e886115 introduced some tests that intend to iterate over strings
that contain definitions of invalid source code and then assert that the
parser does something sensible with these sources.
However, there was a typo in e886115 causing the tests to not execute as
expected: the invalid sources are iterated in the `source_ucli`
variable. However, the parsing is done against the `source_ucl` variable
which contains a known parsable source string!
@c-w

This comment has been minimized.

Copy link
Contributor

c-w commented Oct 15, 2016

@Nurdok: Addressed the latest set of comments, added the fix to the release notes and did some commit squashing because the pull request was getting rather long.

@@ -11,6 +11,10 @@ Major Updates

* Support for Python 2.6 has been dropped (#206, #217).

Bug Fixes

* Made parser more robust to bad source files (#168)

This comment has been minimized.

@Nurdok

Nurdok Oct 15, 2016

Member

Include the pull request as well. i.e., this should be:

* Made parser more robust to bad source files (#168, #214).

This comment has been minimized.

@c-w

c-w Oct 16, 2016

Contributor

Thanks. Added.

@Nurdok

Nurdok approved these changes Oct 16, 2016

@Nurdok Nurdok merged commit cce0cbb into PyCQA:master Oct 16, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Nurdok

This comment has been minimized.

Copy link
Member

Nurdok commented Oct 16, 2016

Merged. Thanks!

@Nurdok Nurdok added the Bugfix label Oct 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment