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

More tests for backslash behaviour #438

Merged
merged 3 commits into from
Dec 17, 2024
Merged

Conversation

tjol
Copy link
Contributor

@tjol tjol commented Dec 16, 2024

Tests for the cases brought up by @eilvelia and @bgotink in #434, as well some more tests for (non-string) escline

@@ -1,2 +1,3 @@
node1
\
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the filename I figured this test was missing something...

Copy link
Member

@bgotink bgotink Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is allowed per spec, but I'm surprised by that.

line-space doesn't contain escline, only node-space has escline
All whitespace between nodes is line-space, so on first sight this test would be invalid, but…
base-node starts with slashdash? type? node-space* string which means this is valid

node1
\
node2

but this isn't

(lorem)node1
\
(lorem)node2

I personally always read the start of base-node as slashdash? (type node-space*)? string, as I interpreted the node-space to be meant as space between the type and name. Apparently that's also how I implemented this, as my parser fails this test atm.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...I noticed this recently in kdl-rs too and it made me wonder whether escline should be part of line-space.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what in the spec allows this, btw?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see:

(type) \
node

is allowed and (type) is optional, therefore

\
node

is allowed.

Quite separately,

/- \
(type)node

and even

/-
\
(type)node

are allowed because /- can always be followed by node-space, even when not in a node.

IMHO, this is too confusing and escline belongs in line-space.

@eilvelia
Copy link
Contributor

Oh, I almost sent a similar PR :)

@@ -0,0 +1 @@
\
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I'm confused: is this actually allowed? I think it should be (just allow all whitespace to be escaped even if it's meaning less) but right now I'm not sure what the spec actually says right now

@eilvelia
Copy link
Contributor

I think a test using non-literal whitespace for prefix could also be useful, e.g.

+++ b/tests/test_cases/input/multiline_string_nonliteral_prefix_fail.kdl
@@ -0,0 +1,5 @@
+node """
+  hey
+ \severyone
+    how goes?
+  """

@zkat zkat merged commit 6906771 into kdl-org:release-2.0.0 Dec 17, 2024
1 check passed
zkat added a commit that referenced this pull request Dec 22, 2024
* Release KDL 2.0.0

* fix grammar for multiline quoted strings to allow escaped whitespace on closing line

* Add unicode-space to raw string

* Remove nonexistent equals-sign from the grammar (#435)

* fix multiline string tests

* grammar: fix disallowed-keyword-identifiers and string-character (#436)

* Back out "fix multiline string tests"

This backs out commit 0c5604b.

* add extra javascript implementation (#437)

* reword interaction multiline + whitespace escape (#439)

* More tests for backslash behaviour (#438)

* More tests for baskslash behaviour

* Incorrect example of escaped final newline

* Test with non-literal indent

* Make line-space a superset of node-space (#440)

* Allow escline everywhere

* escline tests

* Always escape \ inside single quotes in the grammar text (#441)

to match the other uses of it and the metalanguage description below

* Add tests for mandatory whitespace between arguments or properties (#442)

* Add an optional version marker (#444)

* Add version marker to the grammer

* Add version marker to the Changelog

* Update SPEC.md

Co-authored-by: eilvelia <[email protected]>

* add a mandatory newline after the version marker

* add mandatory space between version number

---------

Co-authored-by: eilvelia <[email protected]>

* Fix a changelog line erroneously truncated in #444 (#445)

* fix: move vertical tab to the line-breaking whitespace to match Unicode (#446)

* add vertical tab change test

* final tweaks before release

---------

Co-authored-by: eilvelia <[email protected]>
Co-authored-by: Bram Gotink <[email protected]>
Co-authored-by: Thomas Jollans <[email protected]>
Co-authored-by: Evgeny <[email protected]>
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

Successfully merging this pull request may close these issues.

4 participants