Skip to content

Conversation

@lysnikolaou
Copy link
Member

@lysnikolaou lysnikolaou commented Jun 20, 2020

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

LG assuming the tests pass.

@nnemkin
Copy link
Contributor

nnemkin commented Jun 20, 2020

PyParser_ASTFromXXX functions are public API that don't depend on the parser internals. Why are they being removed? Seems like a pointless compatibility break.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@lysnikolaou
Copy link
Member Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@gvanrossum, @pablogsal: please review the changes made to this pull request.

Copy link
Member

@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

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

LGTM

@pablogsal
Copy link
Member

pablogsal commented Jun 20, 2020

@lysnikolaou I was going to start a PR for re-implementing/re-naming the parser functions but maybe you have already started one? :)

@lysnikolaou
Copy link
Member Author

@lysnikolaou I was going to start a PR for re-implementing/re-naming the parser functions but maybe you have already started one? :)

I was just about to, but you can do it if you think reviewing my PR is going to take more time. Whatever you want!

@pablogsal
Copy link
Member

I was just about to, but you can do it if you think reviewing my PR is going to take more time. Whatever you want!

Go ahead if you have time now, as I will only be able to do one in a couple of hours

@gvanrossum
Copy link
Member

Agreed that the public APIs that don't depend on types like mod_ty from the old parser should stay. I think they should not become macros -- they should stay real functions that just call the Pegen functions. (I don't like the "pegen" name leaking into public APIs.)

@pablogsal pablogsal merged commit 314858e into python:master Jun 20, 2020
@lysnikolaou lysnikolaou deleted the remove-old-parser branch June 20, 2020 18:08
fasih pushed a commit to fasih/cpython that referenced this pull request Jun 29, 2020
Remove some remaining files and Makefile targets for the old parser
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.

6 participants