search

kdl-org / kdl

the kdl document language specifications
https://kdl.dev
Other
1.1k stars 61 forks source link

I *think* escline_comment_node.kdl test is wrong (or the grammar is) #223

Closed tabatkins closed 2 years ago

tabatkins commented 2 years ago

The escline_comment_node.kdl test contains the following:

node1
  \// hey
   node2

and it expects this to parse validly and reserialize out as:

node1
node2

However, my impl is marking it as a parse error (starting at line 2 col 3, the \), and as far as I can tell it's correct to do so. Escaped newlines are from the escline production, and escline only shows up in one place, the node-space production. However, node-space is only allowed between components of a node, not before or after. The space between a node terminator and the start of the following node is only allowed to be linespace, per the nodes production.

I think this is a corner case and not very important besides for completeness, so here are three possible solutions that I don't have a strong opinion between:

I lean slightly towards the third, as it seems the most elegant; linespace is generally meant to be nodespace + line breaks, and it is allowed to be just a single space or similar, so I don't see why an escaped linebreak is problematic for it.

Lucretiel commented 2 years ago

I'll defer to Kat / the community, but I'm actually sort of inclined towards the error here. escline conceptually is for extending a single-line construct (a node and its entities) over several physical lines; there isn't really any meaningful interpretation of that in between nodes.

larsgw commented 2 years ago

See also https://github.com/kdl-org/kdl/issues/121

tabatkins commented 2 years ago

escline conceptually is for extending a single-line construct (a node and its entities) over several physical lines; there isn't really any meaningful interpretation of that in between nodes.

Yeah, I've been a bit back and forth on this, but I find this pretty acceptable. I'll throw up a PR deleting the expected_kdl file, then.

danini-the-panini commented 2 years ago

Gosh darn it, I missed this one! My implementations didn't have a problem with this one because esclines and comments are resolved at the tokenizer level, effectively hiding them from the parser, and therefore are interpreted with no context, therefore they're allowed anywhere