Repository navigation
Update generated lextab/parsetab files - #7174
Conversation
|
Hi there @astrofrog 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃. Everything looks good from my point of view! 👍 If there are any issues with this message, please report them here. |
| lexer = lex.lex(optimize=True, lextab='cds_lextab', | ||
| outputdir=os.path.dirname(__file__), | ||
| reflags=re.UNICODE) | ||
| reflags=int(re.UNICODE)) |
There was a problem hiding this comment.
Ohh, good catch.
I would say both approach is fine with me, but with the aim to get 3.0 out of the door, maybe the latter is better.
There was a problem hiding this comment.
I agree a separate PR with just the minimal fix.
|
Just for info, I opened a PR to ply to make the generated files deterministic (i.e. they will be the same every time they are re-generated): dabeaz/ply#151 |
|
Updating ply to 3.10 actually preproduced the same error from #7068, so since this one passes I'm certain that that issue is solved by this PR. Thanks @astrofrog! |
|
By the way the |
|
@astrofrog - I think, for this case, having the instructions in the files themselves are more useful than having it somewhere in the docs. |
eteq
left a comment
eteq
left a comment
There was a problem hiding this comment.
LGTM aside from one minor change. For #4318 I think we should just have a brief mention in the dev docs that amounts to "look at the header files". I.e., just a sentence or two probably in development/building - if you agree I can just put a commit straight up here adding that.
| # remove it from this folder, then build astropy and run the tests in-place: | ||
| # | ||
| # python setup.py build_ext --inplace | ||
| # pytest astropy/coordinates |
There was a problem hiding this comment.
how about replacing this test part with:
python -c "from astropy.coordinates import angle_utilities;angle_utilities._AngleParser()"
? That seems a bit more fine-grained
There was a problem hiding this comment.
Mine is more general in case we add multiple parsers in future :) And more consistent with what's done for units.
There was a problem hiding this comment.
Hmm, I think it's very dangerous to use the tests to do this... but I see your point that something more general is needed. So I guess this is fine for consistency, but I'm going to make an issue to try to address this better anyway.
| # remove it from this folder, then build astropy and run the tests in-place: | ||
| # | ||
| # python setup.py build_ext --inplace | ||
| # pytest astropy/units |
There was a problem hiding this comment.
This one I don't see an obvious way to replace it with a more targeted call...
[docs only]
Update generated lextab/parsetab files
This does the following:
clean_parse_tables.pyJust to be clear, this does not use
clean_parse_tables.pyanymore.Now one thing that isn't ideal is that in Python 3, it's more obvious that e.g. sets are returned in a non-deterministic order, so these files will change every time they are re-generated, which is not ideal. Some of it is under our control (see the addition of
sortedfor one of the sets), but I'm not sure how to fix them all. This may be something that has to be fixed in ply itself.I think this should fix #7068