-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Fixes #2981 #2985
Fixes #2981 #2985
Conversation
@@ -72,6 +72,11 @@ grammar = | |||
# The **Root** is the top-level node in the syntax tree. Since we parse bottom-up, | |||
# all parsing must end here. | |||
Root: [ | |||
o 'RootContent EOF' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd really love if we could remove parts of this file, instead of adding more...
@michaelficarra @jashkenas Are you fine with EOF
being last of each Root
items? sigh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was pleasantly surprised by the conciseness of this grammar, given the freedom of the syntax the language provides. But of course, keeping grammar to minimum is always favorable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @Nami-Doc that it's not exactly a good solution. Can we not come up with anything better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what a more concise grammar would look like. Even doing a lot more things ;). Kudos to satyr, as usual.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@satyr's use of ditto
in that file is really silver-tongued.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, the stateful implementation is awful. I would prefer something more declarative. Maybe a block where rules within it have the given action.
ruleBlock = (action, block) -> block action
ruleBlock ((a, b, c) -> some action involving a, b and c), (action) ->
o 'other rules', action
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congratulations, now you can't read a single thing anymore :/. I don't think that'd improve anything.
Your function is just a do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was trying to implement a let
, and I forgot we have do
which is the almost equivalent.
do (sharedAction = (a, b) -> some action involving a and b) ->
o 'other rules', sharedAction
o 'more rules', sharedAction
edit: The point is being explicit rather than implicit. It might hurt the eyes, but it still improves readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And we need concat
s because there are arrays, and not every single combination in the block share the same action
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jashkenas what's your stance on this ?
On this PR? Iffy. Wouldn't it be simpler to strip the EOF before parsing, instead of forcing it? On a stateful implementation of |
I don't understand what you mean |
The EOF is added before parsing, to make sure the parser goes through all the tokens. Is there a simpler way to enforce that? |
Alternative got merged long time ago. |
To fix #2981, an example of going the way of checking for EOF, which probably should be done, to avoid problems such as #2984 (this PR includes that fix).
One thing to watch out for is that someone might be relying on generated tokens and this obviously adds the
EOF
token to the list, changing the compiler's output.(parser compiled with Jison 0.4.4)