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

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

Open
larsgw opened this issue Oct 6, 2024 · 13 comments
Open

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

larsgw opened this issue Oct 6, 2024 · 13 comments

Comments

@larsgw
Copy link
Contributor

larsgw commented Oct 6, 2024

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
Copy link
Contributor Author

larsgw commented Oct 6, 2024

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
Copy link
Member

zkat commented Oct 6, 2024

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
Copy link
Member

zkat commented Oct 6, 2024

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
Copy link
Contributor Author

larsgw commented Oct 8, 2024

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

eilvelia commented Oct 8, 2024

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
Copy link
Contributor Author

larsgw commented Oct 8, 2024

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

eilvelia commented Oct 8, 2024

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
Copy link
Contributor Author

larsgw commented Oct 8, 2024

Ah I see, that makes sense.

@larsgw
Copy link
Contributor Author

larsgw commented Oct 8, 2024

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
Copy link
Member

zkat commented Oct 10, 2024

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
Copy link
Contributor Author

larsgw commented Oct 10, 2024

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
Copy link
Member

zkat commented Oct 10, 2024

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

@tabatkins
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants