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

Update formal grammar for KDL 2.0 #285

Merged
merged 4 commits into from
Dec 11, 2023
Merged

Update formal grammar for KDL 2.0 #285

merged 4 commits into from
Dec 11, 2023

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented Aug 21, 2022

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.

Copy link
Member

@zkat zkat left a 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.

@zkat
Copy link
Member

zkat commented Aug 21, 2022

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).

@zkat zkat added the breaking This can only be done for the next major version of KDL label Aug 21, 2022
SPEC.md Outdated Show resolved Hide resolved
@CAD97
Copy link
Contributor Author

CAD97 commented Aug 21, 2022

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.

Copy link
Member

@zkat zkat left a 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:

  1. Require spaces before node-children in nodes (which I think is perfectly fine to do?
  2. Adopt the plain-node-space patch in the comment.
  3. Make sure we're treating slashdash as whitespace, period.
  4. Include the plain-node-space change for line-space that @bgotink suggested

How's that sound?

@zkat zkat mentioned this pull request Aug 28, 2022
@zkat
Copy link
Member

zkat commented Aug 28, 2022

Additionally, before merging this, I'd like to see a handful of updated tests in our compliance test suite that cover these corner cases.

SPEC.md Outdated Show resolved Hide resolved
@CAD97 CAD97 changed the title Update formal grammar Update formal grammar for KDL 2.0 Aug 28, 2022
@CAD97
Copy link
Contributor Author

CAD97 commented Aug 28, 2022

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.

@CAD97 CAD97 changed the base branch from main to kdl-v2 August 28, 2022 20:42
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.

Copy link
Member

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?

Copy link
Contributor Author

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>
@tabatkins
Copy link
Contributor

I merged in the "alternate formulation" that uses required-node-space and optional-node-space, as it's exactly what I was going to suggest while reading the first fix attempt. ^_^

@zkat
Copy link
Member

zkat commented Dec 11, 2023

I'm going to merge this, but I kinda want to see what it takes to allow node{} to be legal. We can do that after this, though.

@zkat zkat merged commit eb55930 into kdl-org:kdl-v2 Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This can only be done for the next major version of KDL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spec is unclear about how slashdash comments work
4 participants