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

Replace nullable_visit by NullableVisitor #86

Merged

Conversation

phorward
Copy link
Contributor

This introduces the NullableVisitor from CPython's pegen.


This is part of the former PR #85.
I was able to run the test-suite locally, using pipenv and python38.
Generally, should we not upgrade to latest python310 with all pegen development?

pablogsal
pablogsal previously approved these changes Mar 24, 2023
@phorward phorward changed the title Replace visit_nullable by NullableVisitor Replace nullable_visit by NullableVisitor Mar 24, 2023
This introduces the NullableVisitor from CPython's pegen.
@pablogsal
Copy link
Contributor

@phorward please don't force push after an approval because it forces us to dismiss the review and review again everything :)

@phorward
Copy link
Contributor Author

@phorward please don't force push after an approval because it forces us to dismiss the review and review again everything :)

I'm sorry, I did only correct the commit message, as it was wrong.

@phorward phorward requested a review from pablogsal March 25, 2023 17:59
@phorward
Copy link
Contributor Author

Hello @pablogsal, I hope you're doing well.

Can you please take another review and maybe merge on this pull request? I've started to work on a pull request setting-up on this one, to successively transfer all overrides from the CPython version of pegen to the stand-alone project. It is important for me to have the stand-alone pegen in similar code-base as the CPython version, to start with further developments on it.

I would like to share my vision of pegen in some kind, if interested, as it may become a universal PEG-like parser generator not only limited to Python. Please contact me, or provide a way to share ideas.

Copy link
Contributor

@lysnikolaou lysnikolaou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@lysnikolaou
Copy link
Contributor

@phorward Thanks for the PR and sorry for taking so long on this. We've been very busy with other things and this fell further down in our list of to-dos.

If you want to start a discussion on ideas you have for the project, an issue would be the best way forward. Please open one, and I'd be happy to get a conversation going there.

@lysnikolaou lysnikolaou merged commit a99d173 into we-like-parsers:main May 4, 2023
@phorward
Copy link
Contributor Author

phorward commented May 5, 2023

@phorward Thanks for the PR and sorry for taking so long on this. We've been very busy with other things and this fell further down in our list of to-dos.

No problem, thanks for merging. Can you please check the GitHub action job that is currently pending, so that the merge becomes complete?

If you want to start a discussion on ideas you have for the project, an issue would be the best way forward. Please open one, and I'd be happy to get a conversation going there.

Ok thanks. I wrote some ideas down on a vision for pegen in my fork anypeg, but I think it'll be the best to divide it into separate issues to easier maintain and focus on.

@phorward phorward deleted the cpython-to-pegen_nullables branch October 4, 2023 20:28
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.

3 participants