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

Empty string should be quote #41

Closed
antoyo opened this issue Nov 6, 2016 · 11 comments
Closed

Empty string should be quote #41

antoyo opened this issue Nov 6, 2016 · 11 comments

Comments

@antoyo
Copy link
Contributor

antoyo commented Nov 6, 2016

Hello.
I think I found an issue with my pull request #39:
I think that the empty string should be quoted, because otherwise, it will be parsed as a Unit.
Thanks.

@chyh1990
Copy link
Owner

Will 2d6a83d be enough?

@antoyo
Copy link
Contributor Author

antoyo commented Nov 10, 2016

I'll make some tests to check if trim() is needed.
Thanks.

@antoyo
Copy link
Contributor Author

antoyo commented Nov 10, 2016

I made some tests and quotes are also needed for a string with only spaces.
So I don't know exactly how it is parsed, but perhaps quotes are also needed when there are spaces at the start/end of a string.
In any case, your commit is not sufficient:
the case of a string like " " will be emitted without quotes and the parser will try to parse it as a Unit.

@chyh1990
Copy link
Owner

chyh1990 commented Nov 11, 2016

I found a reference implementation here: https://github.com/adamkiss/LibYAML/blob/master/yaml/Escaper.php

We are also missing the single quoted part.

I think adding TAB and SPACE to the require quoted character is OK.

@antoyo
Copy link
Contributor Author

antoyo commented Nov 11, 2016

I don't think adding SPACE to the required character is what we want.
For instance, if we have the string "my string", it can be emitted without quotes in YAML.
I don't know the precise rules about the spaces however. Do we need quotes when the string starts/ends with space?

@chyh1990
Copy link
Owner

Yeah, I get your point. But I think quoting something like "my string" is not a bad idea (the php version do this) and more robust. I'm not sure whether just quoting string starts/ends with space will introduce some corner cases?

@dtolnay
Copy link
Collaborator

dtolnay commented Nov 11, 2016

I would strongly prefer not to quote strings unless necessary, even if it means we need to iterate on the heuristic a few times to get it right. Quoting for leading/trailing whitespace seems like a good start.

@chyh1990
Copy link
Owner

It looks good e52b058 ?

@dtolnay
Copy link
Collaborator

dtolnay commented Nov 14, 2016

e52b058: No need to check for starts_with or ends_with '\t' because any string containing '\t' anywhere will be quoted. Just check for ' '.

@antoyo
Copy link
Contributor Author

antoyo commented Nov 14, 2016

A style comment:
no need to use return in the last statement: just omit the semi-colon.

@chyh1990
Copy link
Owner

Closed by #46

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