kdl-org / kdl

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

optional-node-space around equals sign in properties #401

Open larsgw opened 2 weeks ago

larsgw commented 2 weeks ago

The grammar for a property looks like this at the moment:

prop := string optional-node-space '=' optional-node-space value

However, the text says:

A Property is composed of a String, followed immediately by an equals sign, and then a Value.

Probably the latter needs to be reworded to allow whitespace, esclines, and slashdash comments.

(Sidenote, I'm a bit unsure if slashdash comments are good in this context. Firstly, node prop= /-"foo" "bar" would be equivalent to node prop="bar" which is not very obvious (it looks like "foo" and especially "bar" are just Values to me on first sight). Secondly, node prop=/-"foo" "bar" is invalid which seems a bit inconsistent, I feel like that would be clearer. ~Thirdly, I think node prop /-="foo" ="bar" is probably very annoying to parse, especially now prop is a valid Value.~)

larsgw commented 2 weeks ago

Also, I think node prop /-foo = bar is ambiguous, either a Node with a Value "prop" or with a Property prop=bar. Given that, we might need to change the grammar as well.

zkat commented 2 weeks ago

I think a good test for where /- should go is whether we would also allow comments in there.

Would prop = /-foo bar not make sense, but `prop = /foo/ bar would? Why? I feel like the latter is very sensible (and imo, so is the former)

zkat commented 2 weeks ago

The latter is definitely a bigger concern. I'm not sure what to do about that ambiguity: you definitely should be able to slashdash entire props...

larsgw commented 1 week ago

Maybe a better test for where /- should go, is whether the entity you're commenting out could occur there without being commented out. i.e. you already cannot put a slash-dashed Node between two Values, or between a Value and a Children block. So I would suggest no slash-dash before a = in a prop, and after a = only allow slash-dashed Values, not Properties. I would also prefer to remove the mandated whitespace between = and /- in the current grammar.

Does this work?

prop-value-space := plain-node-space* ('/-' plain-node-space* value plain-node-space+)*
prop := string plain-node-space* '=' prop-value-space value

Would prop = /-foo bar not make sense, but prop = /*foo*/ bar would? Why? I feel like the latter is very sensible (and imo, so is the former)

I think prop=/-foo "bar" would be confusing, but I thought prop=/-foo was allowed (it's not, yet), and with spaces it looks okay (in KDL v1, spacing around = was disallowed).

eilvelia commented 1 week ago

It makes sense that an entity next to /- is parsed as it normally would be, but the /- modifier then makes it fully "discarded", same as #_ works in edn (and clojure). This seems conceptually simple and also has no parsing ambiguities. I think it was similar in KDLv1 but later has been changed?

larsgw commented 1 week ago

What would be the "entity next to /-" though in the case of node prop /-foo = bar? It could be either foo or foo = bar, at least in the current grammar.

eilvelia commented 1 week ago

I mean that it would be parsed as if /- is not there, i.e. node prop foo = bar, which can only be <node-name "node"> <value "prop"> <property "foo" "bar">. Then the /- modifier is applied to the property (say, in a semantic action) and this property is not added to the AST. (But the current grammar is ambiguous, yes.)

edit: That is, I view /- just as an entity modifier in the grammar, which alters the semantics of the document.

larsgw commented 1 week ago

Ah I see, that makes sense.

larsgw commented 1 week ago

That explains why node prop=/-foo bar does not make as much sense to me then. Without /- it'd be <node><property "prop" "foo"><value "bar"></node>, making the effect of /- to be "remove the second part of <property> and replace it with the subsequent <value>".

zkat commented 1 week ago

So thinking about it, what we want is for slashdash to work in exactly three places:

  1. On a node
  2. On a node children block (before the {)
  3. Before an entire node entry. That is, if applied to a prop, it has to precede the ENTIRE thing, including a type specifier. Ditto for arguments.

Does that sound like what we want? It seems to be a pretty straightforward rule

larsgw commented 1 week ago

That sounds good to me.

Are we okay with slashdashed children blocks in a non-final position? E.g. node foo /-{ node; } bar is currently allowed, I believe.

zkat commented 1 week ago

idk. Personally, I'm willing to live with that, even though it's a bit weird.

tabatkins commented 1 day ago

If it was easy to block I'd prefer to not allow that, but I'm fine with allowing it if needed, yes.