-
-
Notifications
You must be signed in to change notification settings - Fork 66
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
Update formal grammar for KDL 2.0 #285
Conversation
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.
Honestly I really like how much this simplifies the slashdash implementation, which was kind of tricky before.
The formal grammar is actually the "authoritative" version here, rather than the prose, so this is basically a breaking change. I think it's about time we start talking about the next version of KDL, though. We have a few breaking changes in the pipeline that have just been sitting on the sidelines (mostly minor things, tho). |
Also, it's probably worth doing a survey of existing implementations to see what exactly each of their behaviors here is. It's not unlikely there's divergence here. |
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.
So I guess in summary, there's a few things to change here before we mark it for KDL 2.0:
- Require spaces before
node-children
in nodes (which I think is perfectly fine to do? - Adopt the
plain-node-space
patch in the comment. - Make sure we're treating slashdash as whitespace, period.
- Include the
plain-node-space
change forline-space
that @bgotink suggested
How's that sound?
Additionally, before merging this, I'd like to see a handful of updated tests in our compliance test suite that cover these corner cases. |
I'll bring in some conformance tests for these edge cases later this week; I'm writing a bunch as part of writing my implementation anyway. |
SPEC.md
Outdated
was valid. Now, whitespace is required before a children block, the same as | ||
before arguments and properties. | ||
- `/-` comments on nodes must also be separated by plain (non-`/-`) whitespace. | ||
|
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.
Can you put these in the new CHANGELOG.md file?
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.
Will do later this week along with the conformance tests (I've been doing this in the web UI so far)
Co-authored-by: Christopher Durham <cad97@cad97.com>
I merged in the "alternate formulation" that uses |
I'm going to merge this, but I kinda want to see what it takes to allow |
Closes #284 by making the formal grammar match the implementation and prose, explicitly treating slashdashed nodes as line-space and slashdashed node properties, arguments, and children as node-space.