Skip to content

Fix #3. Use { and } for explicit grouping #21

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

Merged
merged 1 commit into from
Jan 30, 2017

Conversation

stasm
Copy link
Contributor

@stasm stasm commented Jan 26, 2017

This only changes the EBNF grammar and doesn't change the ASDL. The grouping is supposed to help the parser in potentially ambiguous situations. Once the parser figures out the parse tree, the final AST will be unambiguous.

@stasm stasm requested a review from zbraniecki January 26, 2017 21:07
| member-expression
| call-expression
| placeable
Copy link
Collaborator

Choose a reason for hiding this comment

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

why placeable?

key = { { $foo } ->
  [one] Value
}

is that intended to work?

Copy link
Contributor Author

@stasm stasm Jan 26, 2017

Choose a reason for hiding this comment

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

Yes. It's not what the serializer would normally serialize to, but I don't think we need to artificially limit the syntax here. It doesn't change the meaning compared to just using $foo, just like (1) === 1 in JavaScript.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So, what's the use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The use case is tightly related to #4. In particular, we need a way to remove the ambiguity from call-expressions with multiple arguments, some of them being select-expressions:

key = { LIST(foo, { bar, baz -> 
        [bar, baz] Bar and Baz
    }) }

Copy link
Collaborator

@zbraniecki zbraniecki left a comment

Choose a reason for hiding this comment

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

r+

@stasm
Copy link
Contributor Author

stasm commented Jan 30, 2017

Thanks for the review. After talking to @Pike last Friday I partly changed my mind about ASDL. The placeable type shouldn't be part of the ASDL, still, but I think it makes sense to separate the following two concepts:

  • expressions which can be selectors in a select-expression,
  • expressions which can be inside of placeables.

I updated the PR to reflect that. It should map closely to the data structures we'll have in the Rust implementation. @zbraniecki, this probably doesn't need another review from you, but I wanted to let you know before I merge.

@stasm stasm merged commit d8ff7fb into projectfluent:master Jan 30, 2017
@stasm stasm deleted the 3-group branch January 30, 2017 17:00
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.

2 participants