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

Added test for subexpressions #124

Closed
wants to merge 1 commit into from
Closed

Added test for subexpressions #124

wants to merge 1 commit into from

Conversation

Nthalk
Copy link

@Nthalk Nthalk commented Jan 9, 2014

@machty
Copy link
Owner

machty commented Jan 9, 2014

Wanna take a stab at the implementation? :) :) :) :)

@Nthalk
Copy link
Author

Nthalk commented Jan 9, 2014

Machty, I've loved reading your blog about writing parsers to just to learn about it, however, I haven't the first idea of how to start. Is this low hanging fruit that I can attempt?

Given that this is your playground, could you give me a basic idea about what needs to be done or where I could start?

@Nthalk
Copy link
Author

Nthalk commented Jan 9, 2014

I imagine I would want to detect ('s and match them with )'s and leave everything there for the Handlebars compiler?

@machty
Copy link
Owner

machty commented Jan 9, 2014

@Nthalk you would need to start here

https://github.com/machty/emblem.js/blob/master/src/grammar.pegjs#L322

which defines the grammar for the inside of a mustache, and, therefore, the inside of a sexpr. You can trace that rule down to its component rules, such as what a param or hash param can be, and add a subexpressions rule which is essentially an inMustache wrapped in parentheses, and in such cases, you'd need to return an AST.SexprNode, which is the new node I added in my handlebars work.

Honestly, this is a perfect and satisfying PR for you to work on and wrap your brain around PEGjs; I think the bulk of the challenge is just learning PEG and not so much anything specifically nitpicking or annoying about this particular change, so I definitely encourage you to go for it.

@Nthalk Nthalk closed this Jan 25, 2014
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