-
-
Notifications
You must be signed in to change notification settings - Fork 65
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
Release KDL 2.0.0 #434
Release KDL 2.0.0 #434
Conversation
heh. The draft.8 tests killed kdl-rs multiline parsing and I realized the grammar wasn't actually allowing escapes on the closing multiline quoted string line. so there's tests for that too now. |
@tabatkins @tjol I regret to inform you that the Really Complicated whitespace-only multiline string test was actually wrong (I'm pretty sure). The prefixes didn't match, and prefixes need to match exactly before any normalization or empty line collapse, unless the line is completely empty. That is, this should fail, I believe, wince line 2 isn't completely empty, but its prefix doesn't match line 3:
|
@zkat I thought we'd established that the rule that empty lines can contain “any” whitespace takes priority over the prefix requirement. |
Ohhh. Uggghhhhhhhhhhhh ok I'll roll this back |
I think the interactions of multiline strings with escapes are pretty weird currently.
IMO having non-literal whitespace in the prefix-defining line also feels pretty weird. Dedented strings are mostly for readability in the source code, transforming the indentation goes against that. Swift's multiline strings are quite similar, and I think can be an inspiration for kdl's strings. Particularly, the final line (or any ws prefix) there can consist of literal whitespace only, the newline escape is not allowed as the last The "any whitespace in non-content lines" rule forms an exception to the prefix being identical (or at least missing) in all lines, which is also isn't good I think and more difficult to implement (checking the whole line for any whitespace/content instead of only comparing the first characters to the intended prefix). I initially missed this rule when I was updating the ocaml implementation. |
@eilvelia As far as the Swift rules you mention: it does seem like those would simplify the rules for multi line strings. They would be more limiting but these corner cases would be way less confusing to think about by just… banning them altogether (and simplifying parsing) I’m curious what others think. |
I think you misunderstood me.
(That would be the most intuitive, also what Swift does and what the grammar and spec currently suggest, as I think.) |
Well, per this line, the escapes (other than ws) are resolved after dedenting:
|
@eilvelia what that is intended to mean is that:
is an invalid string (that is, Additionally:
Is certainly allowed, and certainly is passing my tests right now. The closing |
Yes, well, that's what should happen, I think the spec is somewhat vague there. That line in particular seemingly suggests that all escapes except ws are unprocessed in case dedent has not been done yet. IIRC the spec only says how escapes are resolved, not how they are lexically analyzed. edit: To add a small clarification, scanning and resolving can often be combined into a single step. In kdl, this is possible for single-line strings (and trivially raw multi-line strings) but not for quoted multi-line. Although an implementation that doesn't scan non-ws escapes beforehand should even pass most (all, I think) current tests. |
@eilvelia do you have any thoughts on what kind of rewording would be helpful here? |
SPEC.md
Outdated
When processing a Multi-line String, implementations MUST dedent the string | ||
_after_ resolving all whitespace escapes, but _before_ resolving other backslash | ||
escapes. Furthermore, a whitespace escape that attempts to escape the final | ||
line's newline and/or whitespace prefix is invalid since the multi-line string |
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'm confused by this phrasing. It seems to say that escaped newline and/or whitespace are not allowed as the multi-line string has to be valid after the escaped whitespace is removed, but the text below and two of the new tests actually allow escaped whitespace in the final line.
The examples below seem to imply that an escaped newline and/or whitespace are only not allowed if the multiline string would become invalid, but that's not how I read this sentence.
Note that would also allow escaped newlines in some cases, e.g.
node """
lorem
ipsum
\
"""
which would be equivalent to
node """
lorem
ipsum
"""
which would imo be worth adding as test.
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.
@bgotink this test sort of tests that but having a more explicit one would be good.
I believe the bit that says that a trailing \
is invalid actually intended to refer to a case like this:
"""
lorem
ipsum\
"""
which would be invalid because:
"""
lorem
ipsum"""
This should probably be reworded, assuming these are the semantics we want to keep.
I'm not terribly inclined to change multiline strings any further, though, tbh.
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'm not terribly inclined to change multiline strings any further,
Oh no, I'm definitely not asking to change multiline strings! I was just confused about the text vs tests.
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.
oh no that's fine. I was also mostly responding to the ongoing convo with @eilvelia who brought up a lot of good points, though I'm leading towards "clarify, don't change"
I'm pretty happy with where this is at right now. I still need to update kdl-rs with these last couple of changes to make sure everything looks good, but barring any other issues getting brought up, I intend to merge this PR and tag the official 2.0.0 sometime on Saturday. /cc @larsgw and @tabatkins who I don't think have responded here yet, wanna make sure y'all see this. |
I felt like I could not give meaningful feedback without attempting to implement it, but I have unfortunately been very busy with my studies the past few weeks. I don't know if I will before Saturday, but that's fine with me. |
hmmmmmmmmmm I was thinking about The only way to represent this is to do either:
Which feels really strange? I know it's a relatively minor thing, but it leaves a bad taste in my mouth for something to be, actually, unrepresentable in one of our string syntaxes. |
That's not the only string you can't represent with raw strings. Another example is |
oh that's a good point. And it does make me feel better. 🤔🤔🤔 |
Isn't it a little weird that newlines are allowed after |
That’s intentional! |
That is: I thought it would be good for /- to mean “slurp up any and all whitespace until the item being commented out”, as opposed to giving it special rules within nodes. It’s more for simplicity (a single /- definition) than thinking this is a thing that’ll be done all the time. We used to have much more strict locations for /- and it turned out to actually complicate grammars and parsers more than it was worth |
Well, currently slashdash is a node/children/arg/prop "modifier" (as I described in #401 (comment)) and can only be inside a node (including at the beginning of it), changing its behaviour would be as simple as multi-line-comment := '/*' commented-block
commented-block := '*/' | (multi-line-comment | '*' | '/' | [^*/]+) commented-block
-slashdash := '/-' line-space*
+slashdash := '/-' node-space*
// Whitespace |
I definitely want the following to be legal:
so I’m not terribly inclined to change this at this stage |
Final heads up: I'm gonna be wrapping up some stuff with kdl-rs this morning, and then releasing and then I'm gonna merge this and release/announce both at the same time. I'd you'd like to tag your own implementation today and have me announce it please lmk! Looks like we're all set here :) |
@zkat ckdl 0.2.1 is out now with opt-in support for KDL 2.0.0 as it stands. I'm just changing the defaults to KDL 2.0 now. Should be done in no time at all. Planning to call that version ckdl-1.0! |
to match the other uses of it and the metalanguage description below
I've tagged version 0.2.0 of npm package |
No huge surprises in the process of changing the defaults, ergo: ckdl-1.0 is released. This version supports both KDL v1 and v2; hybrid mode is the default for reading, and KDL v2 is the default for writing. Feel free to mention in the main announcement. |
* Add version marker to the grammer * Add version marker to the Changelog * Update SPEC.md Co-authored-by: eilvelia <[email protected]> * add a mandatory newline after the version marker * add mandatory space between version number --------- Co-authored-by: eilvelia <[email protected]>
LFGGGGGGG |
Thank you everyone!! Great job!! We did it!!! |
This is it, folks! I think we're ready to finalize 2.0, after something like 3 years of work.
I'm very happy with the language we've defined here, and how nice it feels to use. I want to express my thanks to everyone who has contributed ideas and discussion and work to its specification, all the various implementers who have helped validate it, and all the users of 1.0 who helped give real, experience-based feedback on the previous version of the language, helping us figure out what could be improved.
I'm gonna give folks a few days to review things and make a decision, then I'm hoping to have consensus from at least a few of the major contributors and implementers before publishing this.
Thanks again, everyone. This wouldn't have been possible without you, and I think we've ended up with a real, shining gem.
NOTE: If you're "just" a community member, your opinion is still welcome, although contributors/implementers will likely be prioritized at this point.