Upgrade extern PLY version to 3.9 #5526
Conversation
|
The errors in both travis and appveyor are puzzling. Not sure if they are red herrings or not. With the new version, should one ensure that new lextab/parsetab get made? (e.g., in |
|
Travis is very much puzzling as it passed on my fork here: https://travis-ci.org/bsipocz/astropy/builds/179894615 (except osx that I cancelled due to the traffic jam). |
|
@bsipocz - I decided to restart the failed travis jobs, just in case they were due to some glitch. Though especially the one with coverage enabled looks very suspicious, as it has an internal error with ... actually, the appveyor error on python3 is the same as that failed coverage job, so this is almost certainly related. Regenerating the unit format parsers may actually be needed. |
bsipocz
force-pushed
the
extern_update_ply_3.9
branch
from
4db67e6
to
1b1ad79
Compare
|
Good catch @mhvk, now I've updated the parsing tables, too. |
| @@ -41,8 +41,6 @@ | |||
| import os | |||
| import inspect | |||
|
|
|||
| from ...extern.six.moves import zip | |||
There was a problem hiding this comment.
ah, haven't noticed this was an external package when I added this. Could you re-add this?
There was a problem hiding this comment.
or should I rather make a request on PLY itself?
There was a problem hiding this comment.
I think it's better to add this upstream, especially that this is purely stylistic change, they have a try/except workaround.
There was a problem hiding this comment.
what do you mean by try / except workaround?
There was a problem hiding this comment.
L45 below, they look for the python version for handling the strings, and for the zip it's not necessary either.
There was a problem hiding this comment.
I agree it's not necessary - but faster and more memory-efficient. 😄
There was a problem hiding this comment.
Still, I would rather follow the policy to keep the external packages untouched here, but it would be great if you could contribute any improvements upstream.
| @@ -67,10 +67,8 @@ | |||
| import base64 | |||
| import warnings | |||
|
|
|||
| from ...extern.six.moves import zip | |||
|
@eteq, @astrofrog - Travis failures are due to the glitch on Friday, so this should be good to go, too. |
mhvk
added
the
external
|
This looks all OK, so will merge. I also made a new "external" label for it... |
|
I just rebased #5486 on master, but something is odd. When I run |
|
Oops -- I think those should have been included here. @bsipocz - would you be able to do a quick PR with those?? Thanks... |
|
@taldcroft @mhvk - This is weird as I generated them the same way as the other tables. Also a bit weird why it picks up the |
|
I'm afraid I can't reproduce it, running the tests on master doesn't generate any new files for me. Now running |
|
@taldcroft - By any chance you run the tests with python2? I see the tables generated on travis only on the two python2 builds. |
|
Yes, it was on Python 2. |
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`.
Note this PR updates to exactly have the PLY released version and thus also reverts the six usage changes from #5259 (assuming that the policy is to keep the external packages unchanged whenever possible).