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

Prevent PLY to overwrite its generated files. #5728

Merged
merged 3 commits into from Jan 25, 2017
Merged

Prevent PLY to overwrite its generated files. #5728

merged 3 commits into from Jan 25, 2017

Conversation

@saimn
Copy link
Contributor

saimn commented Jan 23, 2017

Ref #5581, fixes #5726.

After #5526 updated PLY to 3.9, some files were re-generated at runtime
(astropy/coordinates/{angle_lextab.py,angle_parsetab.py}). This is because of
this line https://github.com/dabeaz/ply/blob/master/ply/lex.py#L901 which
assumes that the lextab parameter in lex.lex is a str only, not unicode, which
leads to some surprise when unicode_literals is used on Python 2 ! Then the
module is not imported, so it is regenerated.

So this commit just wraps the lextab value to make sure that a str is always
passed to lex.lex. And it also reverts #5563 as wrapping in a try/except is no
more necessary.

cc @bsipocz @eteq @mhvk

Ref #5581, fixes #5726.

After #5526 updated PLY to 3.9, some files were re-generated at runtime
(astropy/coordinates/{angle_lextab.py,angle_parsetab.py}). This is because of
this line https://github.com/dabeaz/ply/blob/master/ply/lex.py#L901 which
assumes that the lextab parameter in `lex.lex` is a str only, not unicode, which
leads to some surprise when unicode_literals is used on Python 2 !

So this commit just wraps the lextab value to make sure that a str is always
passed to `lex.lex`.
@mhvk

This comment has been minimized.

Copy link
Contributor

mhvk commented Jan 23, 2017

Great find! I was very confused indeed by why we needed the try/except. Could you add a comment for each of the lines? Something along the lines of

# PY2: need str() to ensure we do not pass on a unicode object.

This will also remind us we can remove this work-around for astropy 3.0...

@mhvk

This comment has been minimized.

Copy link
Contributor

mhvk commented Jan 23, 2017

@bsipocz - Could you have a look too? You understand this better than I do...

@pllim

This comment has been minimized.

Copy link
Member

pllim commented Jan 23, 2017

Also need a change log.

@bsipocz

This comment has been minimized.

Copy link
Member

bsipocz commented Jan 23, 2017

Great investigating @saimn, thanks! It looks good to me.

@saimn

This comment has been minimized.

Copy link
Contributor Author

saimn commented Jan 23, 2017

@mhvk - Done ! Also added the changelog entry.

@mhvk

This comment has been minimized.

Copy link
Contributor

mhvk commented Jan 25, 2017

OK, all looks great now, so merging (sorry for the bit of a delay).

@mhvk mhvk merged commit 7be08b0 into astropy:master Jan 25, 2017
1 check passed
1 check passed
ci/circleci Your tests passed on CircleCI!
Details
@saimn saimn deleted the saimn:fix-ply branch Jan 25, 2017
bsipocz added a commit that referenced this pull request Feb 14, 2017
Prevent PLY to overwrite its generated files.
@bsipocz

This comment has been minimized.

Copy link
Member

bsipocz commented Feb 28, 2017

This can probably be backported to 1.0.12, but it's not a clean cherry-pick, so I leave as is for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.