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

Clarify line-ending backslash in multi-line strings #728

Closed
goodmami opened this issue Apr 16, 2020 · 9 comments
Closed

Clarify line-ending backslash in multi-line strings #728

goodmami opened this issue Apr 16, 2020 · 9 comments

Comments

@goodmami
Copy link
Contributor

goodmami commented Apr 16, 2020

The v1.0.0-rc.1 spec says, regarding multi-line strings:

When the last non-whitespace character on a line is a \, it will be trimmed along with all whitespace (including newlines) up to the next non-whitespace character or closing delimiter. All of the escape sequences that are valid for basic strings are also valid for multi-line basic strings.

This is unclear for the case when that last \ is escaped. I think the ABNF is correct and when I tested with tomlkit it looks good:

>>> from tomlkit import parse
>>> parse(r'''
... ml-escaped-nl = """
...   foo \ 
...   bar \\ 
...   baz \\\ 
...   quux"""''')
{'ml-escaped-nl': '  foo bar \\ \n  baz \\quux'}

But I think the description in the spec can be clearer. I propose the following 1-word change (or 2 if you include the epenthesis of the preceding "a" to "an"):

- ending backslash". When the last non-whitespace character on a line is a `\`, it
+ ending backslash". When the last non-whitespace character on a line is an unescaped `\`, it 

The code snippet above might also be a good test case.

Aside: Is https://github.com/BurntSushi/toml-test the de jure, or even de facto, test set? It hasn't been updated in a while.

@eksortso
Copy link
Contributor

That's a good proposal. Could you open a PR with that change, mentioning #728 in your description?

@eksortso
Copy link
Contributor

Regarding testing, you could ask in #585.

@goodmami
Copy link
Contributor Author

@eksortso no problem. For the PR, shall I also add the test case to tests/hard_example.toml, which already has some tests about multi-line strings? Or is that what you meant by asking in #585?

@eksortso
Copy link
Contributor

@eksortso no problem. For the PR, shall I also add the test case to tests/hard_example.toml, which already has some tests about multi-line strings?

If it would fit, I'd say yes. I wasn't even thinking about that, but good call.

Or is that what you meant by asking in #585?

You were asking about the state of testing. I wanted to keep that separate, which is why I referred you to #585.

@goodmami
Copy link
Contributor Author

Great, that's what I thought you meant but just wanted to clarify.

I've just pushed the wording change and made #729.

I noticed that tests/hard_example.toml actually tested a multi-line array, not a multi-line string, but it did have other string tests. Since the test case didn't fit as well as I'd thought, I left it off the initial PR. I can add a commit adding the test, perhaps as a separate test file for strings?

@eksortso
Copy link
Contributor

Your PR looks good to me. Hoping it gets merged soon. @pradyunsg @mojombo Can we merge #729?

@eksortso
Copy link
Contributor

I'm not even sure about test commits, to be honest. Again, read #585 and bring up this particular issue, and ask where tests should go.

@goodmami
Copy link
Contributor Author

Got it. Thanks!

@pradyunsg
Copy link
Member

Gonna go ahead and close this. Let's keep the testing question separate.

LongTengDao pushed a commit to LongTengDao/TOML that referenced this issue Apr 23, 2020
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

3 participants