kdl icon indicating copy to clipboard operation
kdl copied to clipboard

Do not escape / (Solidus, Forwardslash)

Open danini-the-panini opened this issue 2 years ago • 6 comments

I don't believe / need to be escaped in strings. This may be considered a breaking change now that we've reached 1.0, so another solution might be to allow it to be escaped during parsing, but not to escape it when stringifying.

Closes #194

danini-the-panini avatar Sep 22 '21 06:09 danini-the-panini

This is, unfortunately, a breaking change, so I'm moving it to target the v2 branch.

zkat avatar Sep 28 '21 00:09 zkat

The secondary solution (allow it to be escaped in parsing, but don't require it to be escaped in printing) appears to already be expected by the testsuite; see all_escapes.kdl and its expected output.

(I think superfluous escapes are fine - all it means is we can't give special meaning to an escaped forward slash \/ later - so I don't think we need to change any behavior here at all. But it would certainly be tidier to remove \/, since it does suggest that / would do something special in strings if not escaped.)

tabatkins avatar Oct 05 '21 20:10 tabatkins

oh lol I should look at commit histories; that was changed by the OP several days ago. :+1:

tabatkins avatar Oct 05 '21 20:10 tabatkins

@tabatkins oh, sorry, i made a bunch of alternative fixes, one of which was to do exactly what you've suggested, which got merged 😝

danini-the-panini avatar Oct 06 '21 08:10 danini-the-panini

If an alternate has been merged, can we close this?

hkolbeck avatar Oct 06 '21 20:10 hkolbeck

The alternate that was merged was just a change to the expected serialization for the tests; the larger (breaking change) question of whether to remove the \/ escape entirely is still open as a possible 2.0 change.

tabatkins avatar Oct 06 '21 22:10 tabatkins