search

kdl-org / kdl

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

Bare single-line comments #318

Closed P1n3appl3 closed 10 months ago

P1n3appl3 commented 1 year ago

The current grammar for single line comments (single-line-comment := '//' ^newline+ (newline | eof)) makes it sound like lines consisting of just // (or // with any amount of leading whitespace) wouldn't be recognized as comments.

This KDL treesitter grammar implements that faithfully, but I'm pretty sure most parsers recognize bare // as single line comments. This results in the syntax highlighting being off even though the file parses fine, like in this example from the Zellij example config: the 3rd line has a bare single line comment which isn't highlighted correctly

The spec's grammar should probably updated to say ^newline* instead of ^newline+, unless it's intentional that you need a space or other character after the // for it to be a valid single line comment (in which case existing parsers are probably nonconformant), in which case that should probably be clarified.

P1n3appl3 commented 1 year ago

Update: this was reported as a bug in the treesitter grammar and they've landed a change to treat bare // as a comment. I'm pretty well convinced that existing implementations are doing the right thing here and the spec should be updated to reflect that.

tabatkins commented 1 year ago

Yes, this does appear to be a bug in the grammar. I also appear to have interpreted this as a * rather than a + in my own implementation.

I'll ping @zkat for a final yay/nay, and fix the grammar next week otherwise.

zkat commented 1 year ago

I agree with fixing this. I also thought we were making some grammar changes already to fix this already, but I can't remember where that is, if at all.

tabatkins commented 10 months ago

After saying "next week" I then decided this needed to bake for nine months, apparently. Congratulations, we're all new parents of a bugfix.

tabatkins commented 10 months ago

Whoops, I'd forgotten about the existence of the v2 branch, where this fix is currently baking. Hm, suppose I'll revert for now.