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

text-block ||| fails to allow trailing whitespace before new-line #421

Closed
kardianos opened this issue Dec 13, 2017 · 7 comments
Closed

text-block ||| fails to allow trailing whitespace before new-line #421

kardianos opened this issue Dec 13, 2017 · 7 comments

Comments

@kardianos
Copy link

From the spec, carriage returns are considered white space.

From the spec:

Text block, beginning with |||, followed by optional whitespace and a new-line. The next line must be prefixed with some non-zero length whitespace W. The block ends at the first subsequent line that does not begin with W, and it is an error if this line does not contain some optional whitespace followed by |||. The content of the string is the concatenation of all the lines that began with W but with that prefix stripped. The line ending style in the file is preserved in the string. This form cannot be used in import statements.

Critically, the ||| may be followed by an optional whitespace and then a new-line. As defined by the spec this would allow jsonnet to work with windows or unix line ending styles. However in implementation, both jsonnet and go-jsonnet both require a "\n" right after the "|||". This causes jsonnet files to fail with windows line endings if it contains a "|||" text block.

Other jsonnet features work just fine with CRLF line endings, including mutliline "normal" strings.

(This was discovered by a coworker who had git's auto crlf "feature" enabled.)

@kardianos
Copy link
Author

Related to #289.
This issue is about what appears to be a spec vs implementation discrepancy. #289 is about a proposed feature to strip trailing whitespace. Linking as if someone works on #289 they may wish to take this into account.

@sparkprime
Copy link
Contributor

Thanks for the report, chomping the whitespace after ||| is pretty trivial I think so we should fix this.

Note that converting line endings will change the content of the string specified by |||, as described in the spec. So git's auto crlf feature would still affect the generated jsonnet. I was never comfortable about that, but not sure how to fix it.

@kardianos
Copy link
Author

Note that converting line endings will change the content of the string specified by |||, as described in the spec. So git's auto crlf feature would still affect the generated jsonnet. I was never comfortable about that, but not sure how to fix it.

I'm aware of that. The only two options seem to either (1) leave line endings as written withing multi-line strings or (2) normalize them to new-lines in all multi-line string and text-blocks. I guess the question I would pose, who would complain if "\r" characters went away in JSON output? Is a "canonical" output of jsonnet input of value, regardless of system line endings?

@sbarzowski
Copy link
Collaborator

sbarzowski commented Dec 13, 2017

I think the best way is to always treat line endings in the same way, i.e. treat CRLF in the source as LF. Ideally at the lexer level. I think line ending convention in the source code should never affect the semantics. User's platform also shouldn't affect the output.

EDIT:
@kardianos That's your option (2).

@sbarzowski
Copy link
Collaborator

And there is also a slightly evil option of disallowing CRLF in jsonnet completely to avoid confusion.

The only downside I see is forcing people to set up git not to do crlf conversion for .jsonnet and .libsonnet files. (As I'm pretty sure all reasonable editors on Windows play nicely with LF endings).

@kardianos
Copy link
Author

@sbarzowski I'd also go with (2) as someone who has tried to make sassc output the same on windows and *nix, and has so far failed.

For your (evil 3) option, I'd recommend against it. At least in Go, it used to be stricter about certain line endings and BOM options and finally relaxed it. Though it is always easier to relax then to make stricter.

@sparkprime
Copy link
Contributor

(2) SGTM

It is easy to add some std.unix2dos(||| ... |||) wrapper. Also, we could have a syntax for it like |||d or whatever. YAML already has several flags you can put, including the one referenced in #289

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