Skip to content

bpo-40939: Remove the old parser #20768

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 11, 2020
Merged

bpo-40939: Remove the old parser #20768

merged 3 commits into from
Jun 11, 2020

Conversation

pablogsal
Copy link
Member

@pablogsal pablogsal commented Jun 9, 2020 •
edited by bedevere-bot
Loading

@pablogsal pablogsal mentioned this pull request Jun 9, 2020
Closed
@pablogsal pablogsal force-pushed the remove_old_parser branch 6 times, most recently from c1a2097 to 27a0c11 Compare June 10, 2020 00:29
remilapeyre
remilapeyre reviewed Jun 10, 2020
View reviewed changes
Copy link
Contributor

@remilapeyre remilapeyre left a comment

Choose a reason for hiding this comment

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

Hi, there is also a note in Doc/using/cmdline.rst that will need to be removed.

@pablogsal
Copy link
Member Author

Hi, there is also a note in Doc/using/cmdline.rst that will need to be removed.

Thanks for the catch!

@pablogsal pablogsal marked this pull request as ready for review June 10, 2020 11:25
@pablogsal pablogsal requested review from gpshead and a team as code owners June 10, 2020 11:25
@pablogsal pablogsal changed the title Remove the old parser bpo-40939: Remove the old parser Jun 10, 2020
@pablogsal pablogsal force-pushed the remove_old_parser branch from 62192dd to 3dc1827 Compare June 10, 2020 13:09
lysnikolaou
lysnikolaou approved these changes Jun 10, 2020
View reviewed changes
Copy link
Member

@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.

This LGTM! Amazing work, I couldn't find a single miss! 🚀

Also, I guess this is what we've been really working for in the last months. Feels good!

@pablogsal pablogsal force-pushed the remove_old_parser branch 2 times, most recently from a4055e4 to 738a7ae Compare June 10, 2020 23:58
@pablogsal
Copy link
Member Author

I plan to land this tomorrow as this PR keeps getting merge conflicts from everywhere and it will be easier to fix small things in future PRs if needed.

gvanrossum
gvanrossum reviewed Jun 11, 2020
View reviewed changes
Lib/test/test_peg_parser.py
@@ -1,803 +0,0 @@
import ast
Copy link
Member

Choose a reason for hiding this comment

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

why is this file deleted?

Copy link
Member Author

Choose a reason for hiding this comment

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

This file is comparing the ast of the old parser to the last of the new parser, so it serves no purpose anymore. We have been adding all negative cases (testing extensions) into other files so we won't lose anything

@pablogsal pablogsal force-pushed the remove_old_parser branch 2 times, most recently from e11e688 to 1ad8ff2 Compare June 11, 2020 15:22
@pablogsal
Copy link
Member Author

pablogsal commented Jun 11, 2020 •
edited
Loading

I have rebased again to solve more merge conflicts. Could you review again?

pablogsal added 3 commits June 11, 2020 17:10
@pablogsal
Remove the old parser
51d73f8 
Update cmdline.rst

📜🤖 Added by blurb_it.

Clean some tests and bring back some deleted cases
@pablogsal
Restore tests in test_string_literals
1118380
@pablogsal
Rename parser files
635a9e2
@pablogsal pablogsal force-pushed the remove_old_parser branch from 1ad8ff2 to 635a9e2 Compare June 11, 2020 16:10
gvanrossum
gvanrossum approved these changes Jun 11, 2020
View reviewed changes
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.

I wonder if we could backport the file renames? (Not the moves from Parser/pegen/* to Parser/*, but the rename of parse.c to parser.c and parse_string.c to string_parser.c.)

@pablogsal
Copy link
Member Author

I wonder if we could backport the file renames? (Not the moves from Parser/pegen/* to Parser/*, but the rename of parse.c to parser.c and parse_string.c to string_parser.c.)

👍 I will try to make a PR later today with that

@gvanrossum
Copy link
Member

gvanrossum commented Jun 11, 2020 via email

@pablogsal pablogsal merged commit 1ed83ad into python:master Jun 11, 2020
@pablogsal pablogsal deleted the remove_old_parser branch June 11, 2020 16:30
arun-mani-j pushed a commit to arun-mani-j/cpython that referenced this pull request Jul 21, 2020
@pablogsal @arun-mani-j
bpo-40939: Remove the old parser (pythonGH-20768)
584cc2e 
This commit removes the old parser, the deprecated parser module, the old parser compatibility flags and environment variables and all associated support code and documentation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lysnikolaou lysnikolaou lysnikolaou approved these changes

@remilapeyre remilapeyre remilapeyre left review comments

@gvanrossum gvanrossum gvanrossum approved these changes

@gpshead gpshead Awaiting requested review from gpshead

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@pablogsal @gvanrossum @lysnikolaou @remilapeyre @the-knights-who-say-ni @bedevere-bot