Correctly remove multiline trailing whitespace #103
Conversation
src/value.rs
Outdated
| // Last index | ||
| remove_trailing(text); | ||
| match text.rfind('\n') { | ||
| Some(p) if text[p + 1..].chars().all(|c| c == ' ') => { |
There was a problem hiding this comment.
This does not recognize tabs and such as whitespace, I assume this is intentional.
There was a problem hiding this comment.
Right, really I should say "spaces" and not "whitespace." If the last line has some other whitespace, like a tab character, it should not be removed. This matches the behavior of the reference implementation:
# test.nix
''
hello
'' # this is a tab
$ nix-instantiate --parse test.nix
"hello\n\t"
|
If we do this, we should do it properly from the start, i.e. remove all whitespace prefixes. In fact I have started on a patch for that, but it needs some more polishing, expect a PR ~next week :) |
|
don't we already do that? The only difference necessary is that the first and last line are handled differently (in them, if they contain only spaces |
|
Right, I believe that is already handled. That said, I also think that this entire logic could use a rework. I started by trying to just dumbly port the reference implementation in this commit: darichey@d56b15f I confirmed that it at least parses all multiline strings in nixpkgs (see #107), but honestly the code is kind of ugly, and I haven't done any kind of benchmarks, so I didn't bother making a PR yet. Please feel free to use it as inspiration @Ma27 :) |
Summary & Motivation
Previously, trailing whitespace on the last line of a multiline string was always removed, but this is not necessarily correct. For example, the string
''hello ''should keep its trailing whitespace.This PR attempts to mimic the logic used in the reference implementation of the parser to instead only remove the final line if it consists solely of whitespace.