-
-
Notifications
You must be signed in to change notification settings - Fork 222
Improve Instantiating Grammars documentation with verbose examples #528
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
Improve Instantiating Grammars documentation with verbose examples #528
Conversation
Tests are failing. It's stemming from the code examples. I'll add some markscript checks and run through the test suite locally to ensure that it will pass. Then upload my changes. |
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.
Thanks for this! I think it will make a nice addition to the docs. I'd just suggest simplifying it a bit to keep the signal-to-noise ratio high.
(And sorry about the Markscript stuff, it's a bit of a pain. I implemented it a long time ago as a way of making sure that all the code examples in the docs are actually valid, but it's a bit tricky to understand.)
doc/api-reference.md
Outdated
Arithmetic { | ||
Exp | ||
= AddExp | ||
AddExp | ||
= AddExp "+" PriExp -- plus | ||
| AddExp "-" PriExp -- minus | ||
| PriExp | ||
PriExp | ||
= "(" Exp ")" -- paren | ||
| "+" PriExp -- pos | ||
| "-" PriExp -- neg | ||
| ident | ||
| number | ||
ident (an identifier) | ||
= letter alnum* | ||
number (a number) | ||
= digit* "." digit+ -- fract | ||
| digit+ -- whole |
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.
Since this is the API reference for ohm.grammars
, I think it's best if we reduce this to a minimal grammar, e.g.:
const parentDef = String.raw`
Parent {
start = "parent"
}
`;
With an arithmetic grammar, there's a whole bunch of detail that distracts from what we're trying to show — and a reader likely doesn't now what's relevant and what's not.
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 a good idea. I've greatly simplified the examples in accordance with your instructions and added mark script tests inline to ensure it all works.
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 really like mark script. It's really cool.
doc/api-reference.md
Outdated
// combinedGrammars: | ||
// { | ||
// Arithmetic: object, | ||
// BetterArithmetic: object | ||
// } |
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.
Did you mean to show what childGrammar
is here instead?
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.
Yes. I fixed this.
Please hold off on approving this PR. For some reason my commits aren't attached correctly to my identity. I've gotta figure that out. |
7a4e1a2
to
9f6c173
Compare
Alright I fixed that. Good to go. |
Thanks again! |
Hi Friends.
This PR improves the documentation around Instantiating Grammars somewhat by providing verbose examples. We could simplify the examples to save space, and add clarity, but I think verbose examples are usually helpful.
The desire to add this came about because I was a bit confused about how to actually instantiate a grammar that inherits from another. The documentation is clear, but I had to read it thoroughly a couple of times, and ask for some help, before I understood what was going on.