Skip to content

Prevent PLY to overwrite its generated files. - #5728

Merged
mhvk merged 3 commits into
astropy:masterfrom
saimn:fix-ply
Jan 25, 2017
Merged

mhvk merged 3 commits into
astropy:masterfrom
saimn:fix-ply

Conversation

@saimn

@saimn saimn commented Jan 23, 2017

Copy link
Copy Markdown
Contributor

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 astropy#5581, fixes astropy#5726.

After astropy#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

mhvk commented Jan 23, 2017

Copy link
Copy Markdown
Contributor

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

mhvk commented Jan 23, 2017

Copy link
Copy Markdown
Contributor

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

@pllim

pllim commented Jan 23, 2017

Copy link
Copy Markdown
Member

Also need a change log.

@bsipocz

bsipocz commented Jan 23, 2017

Copy link
Copy Markdown
Member

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

@saimn

saimn commented Jan 23, 2017

Copy link
Copy Markdown
Contributor Author

@mhvk - Done ! Also added the changelog entry.

@mhvk

mhvk commented Jan 25, 2017

Copy link
Copy Markdown
Contributor

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
@saimn
saimn deleted the fix-ply branch January 25, 2017 17:30
bsipocz pushed a commit that referenced this pull request Feb 14, 2017
Prevent PLY to overwrite its generated files.
@bsipocz

bsipocz commented Feb 28, 2017

Copy link
Copy Markdown
Member

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

Development

Successfully merging this pull request may close these issues.

SkyCoord

4 participants