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

bpo-40939: Add the new grammar to the grammar specification documentation #19969

Merged
merged 8 commits into from Jul 27, 2020

Conversation

pablogsal
Copy link
Member

@pablogsal pablogsal commented May 6, 2020

Example of how it looks like when using the new lexer:

grammar

https://bugs.python.org/issue40939

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 worry about some of the rather verbose comments in the grammar file, and the various error rules. Also there are some cases where lookaheads are essential (notably the way formal parameters are now parsed). And maybe the ordering and formatting of some of the rules could be better.

PS. Your initial comments still shows variables (a=...) but the script correctly erases those.

Doc/tools/extensions/peg_highlight.py Outdated Show resolved Hide resolved
@pablogsal
Copy link
Member Author

pablogsal commented May 6, 2020

I worry about some of the rather verbose comments in the grammar file, and the various error rules.

I can try to eliminate alternatives if they start with "invalid_" but I think it will not be super resilient :(

Also there are some cases where lookaheads are essential (notably the way formal parameters are now parsed).

Would you like to bring lookaheads back?

And maybe the ordering and formatting of some of the rules could be better.

Not much we can do here with a custom lexer unless we touch the original file.

PS. Your initial comments still shows variables (a=...) but the script correctly erases those.

Yeah, I removed the variables in the last amend after I realized that we wanted to get rid of them.

@gvanrossum
Copy link
Member

I worry about some of the rather verbose comments in the grammar file, and the various error rules.

I can try to eliminate alternatives if they start with "invalid_" but I think it will not be super resilient :(

Plus, you'd have to eliminate both the alternatives and the rules. But these really do not deserve to be listed in the reference manual.

Also there are some cases where lookaheads are essential (notably the way formal parameters are now parsed).

Would you like to bring lookaheads back?

Only the ones that are significant -- and those in formal parameters are significant (without them, def foo(bar baz) would be legal)

And maybe the ordering and formatting of some of the rules could be better.

Not much we can do here with a custom lexer unless we touch the original file.

Yeah, we can do that later.

@pablogsal
Copy link
Member Author

Only the ones that are significant -- and those in formal parameters are significant (without them, def foo(bar baz) would be legal)

Hmmm.... is there any obvious automatic way (using regular expressions) to detect the ones we want from the ones we don't want? Otherwise, we may need to maintain a copy of the grammar for displaying here and manually trim what we need.

@gvanrossum
Copy link
Member

gvanrossum commented May 6, 2020 via email

@lysnikolaou
Copy link
Contributor

Hmmm.... is there any obvious automatic way (using regular expressions) to detect the ones we want from the ones we don't want?

A heuristic that is currently 100% correct is that a lookahead is only used for optimisation and can be skipped if and only if it is a positive lookahead and it appears first in its alternative. Although this is currently correct, it seems like something that could very easily be invalidated in the future, so I don't know if it can be used in the script.

Copy link
Contributor

@willingc willingc left a comment

Choose a reason for hiding this comment

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

Overall, I like the approach. There are a few places where color makes a line choppy and hard to follow and may make it more difficult to read than just black text.

@gvanrossum gvanrossum changed the title bpo-40334: Add the new grammar to the grammar specification documentation bpo-40939: Add the new grammar to the grammar specification documentation Jul 26, 2020
@gvanrossum
Copy link
Member

I think I'm going to do something with this after all. Let's see if we can't make the highlighter also remove invalid_* rules and comments.

@gvanrossum
Copy link
Member

Okay, @lysnikolaou we may need your review because now it's both Pablo's and my code.

@pablogsal
Copy link
Member Author

Should we backport this to 3.9? (Minus the Grammar/Grammar delete. :-)

I think it makes sense to do the backport. What do you think @lysnikolaou ?

@gvanrossum
Copy link
Member

Okay then @pablogsal what do you think of my version?

Copy link
Contributor

@willingc willingc left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @pablogsal and @gvanrossum.

Copy link
Member Author

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

Okay then @pablogsal what do you think of my version?

I think is great :) I left a minor question, but feel free to ignore

Note: I cannot approve my own PR, so go ahead and merge!

Doc/reference/grammar.rst Show resolved Hide resolved
@lysnikolaou
Copy link
Contributor

Should we backport this to 3.9? (Minus the Grammar/Grammar delete. :-)

I think it makes sense to do the backport. What do you think @lysnikolaou ?

I also think that backporting this to 3.9 makes sense.

@gvanrossum
Copy link
Member

The new version I pushed has a longer intro in grammar.rst, both to explain (some of) what's omitted and to clarify the notation. It also restores most lookaheads, except for the positive ones that are purely optimizations (as a heuristic I use | followed by a positive lookahead). It makes the grammar a little uglier but reduces the possibility of looking like an ambiguity.

@gvanrossum gvanrossum added the needs backport to 3.9 only security fixes label Jul 27, 2020
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. Thanks!!

@gvanrossum gvanrossum merged commit 72cabb2 into python:master Jul 27, 2020
@miss-islington
Copy link
Contributor

Thanks @pablogsal for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @pablogsal and @gvanrossum, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 72cabb2aa636272e608285f5a6ba83b62be9be4e 3.9

gvanrossum pushed a commit that referenced this pull request Jul 27, 2020
…cumentation (GH-19969)

(We censor the heck out of actions and some other stuff using a custom "highlighter".)

Co-authored-by: Guido van Rossum <guido@python.org>.
(cherry picked from commit 72cabb2)

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>
@bedevere-bot
Copy link

GH-21641 is a backport of this pull request to the 3.9 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Jul 27, 2020
gvanrossum added a commit that referenced this pull request Jul 27, 2020
…cumentation (GH-19969) (#21641)

(We censor the heck out of actions and some other stuff using a custom "highlighter".)

(cherry picked from commit 72cabb2)

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Aug 4, 2020
…ation (pythonGH-19969)

(We censor the heck out of actions and some other stuff using a custom "highlighter".)

Co-authored-by: Guido van Rossum <guido@python.org>
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Aug 20, 2020
…ation (pythonGH-19969)

(We censor the heck out of actions and some other stuff using a custom "highlighter".)

Co-authored-by: Guido van Rossum <guido@python.org>
xzy3 pushed a commit to xzy3/cpython that referenced this pull request Oct 18, 2020
…ation (pythonGH-19969)

(We censor the heck out of actions and some other stuff using a custom "highlighter".)

Co-authored-by: Guido van Rossum <guido@python.org>
@pablogsal pablogsal deleted the grammar branch May 19, 2021 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants