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

python.gram: SyntaxError column numbers to match cpython 3.10 #48

Closed
wants to merge 11 commits into from

Conversation

edemaine
Copy link
Contributor

As suggested by @MatthieuDartiailh in this comment, this is a completion of #41 (which includes @MatthieuDartiailh original commits) that includes all the row/column tests, as well as a fix to my issue #47.

@MatthieuDartiailh
Copy link
Collaborator

Looks great ! Thanks for taking the time to do that.

Since the idea is to match CPython it may make sense to have CPython parse the same test code and validate our error syntax against the CPython one. WDYT ?

@cfbolz
Copy link

cfbolz commented Nov 8, 2021

hi everyone! I am considering to improve a few things in python.gram but don't want to tread on anybody's else's effort. Should I help out with the failing tests here?

(I was wondering whether the reason for the test failures is that python_generator.py does not seem to special-case the invalid_-prefixed rules in the same way that CPython's c_generator.py does it?)

@edemaine
Copy link
Contributor Author

edemaine commented Nov 8, 2021

I'd definitely appreciate the help with fixing these tests! Or rather, fixing python.gram so that these tests which should pass actually do pass. (Though not sure whether @MatthieuDartiailh is currently working on it.)

And we should make the comparison to Python only run on 3.10 (and maybe even a specific minor version?). Incidentally, CI should probably be updated to use 3.10.0.

@MatthieuDartiailh
Copy link
Collaborator

I am not working on this at the moment no.

@pablogsal
Copy link
Contributor

should probably be updated to use 3.10.0.

Done in #50

@MatthieuDartiailh
Copy link
Collaborator

If nobody is working on this I will try to pick this up again soon now that my proof of concept for using pegen in a project I maintain is ready. (nucleic/enaml#474).

I will probably first look into using the two passes error reporting mechanism used in the C parser.

@cfbolz
Copy link

cfbolz commented Jan 30, 2022

I didn't get around to it sorry :-(

@MatthieuDartiailh
Copy link
Collaborator

With #41 merged I believe everything this addressed has been fixed so closing. Feel free to reopen if the need arises.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants