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

On Windows, results depend on file lineends used by Git #14

Open
Blaisorblade opened this issue Aug 12, 2016 · 8 comments
Open

On Windows, results depend on file lineends used by Git #14

Blaisorblade opened this issue Aug 12, 2016 · 8 comments

Comments

@Blaisorblade
Copy link

Blaisorblade commented Aug 12, 2016

Values written using neat-interpolation use the newlines of the source file. On Windows that means they'll likely contain \r\n instead of \n. That's unfortunate and unlikely to be intended, so I'd guess neat-interpolation should canonicalize newlines to \n.

Rationale 1: this package is used for string constants, so they better be constant.
Rationale 2: on Windows the actual newlines in a Git-managed project depend on how exactly Git was setup. The setup recommended by Git for Windows converts 2.9.2 them to Windows newlines.
Rationale 3: If I didn't use raw strings, I'd be writing \n in those strings. That's because the conversion to \r\n is done at the lowest possible level (when writing to files opened in text mode).

Use case:
A colleague used neat-interpolation (which is cool) in a test case I'm porting to Windows. However, I got the following:

Failures:

  src/test\Stack\GhciSpec.hs:54:
  1) Stack.Ghci.GHCi, Script rendering, should render GHCi scripts, with one lib                                                                      rary package
       expected: ":add Lib.A\r\n:module + Lib.A\n"
        but got: ":add Lib.A\n:module + Lib.A\n"

The expected value is written with a neat-interpolation quasiquote, and it has a \r\n instead of \n.
EDITED: To confirm my analysis above, I resaved the specific file using Unix line ends, and the tests passed.

I didn't find any explicit mention of newlines in the package, but I could confirm this happens because even on Windows (somewhat sensibly) lines only processes \n, leaving \r alone:

> lines "a\r\nb"
["a\r","b"]
Blaisorblade added a commit to commercialhaskell/stack that referenced this issue Aug 12, 2016
Note the issue is not triggered on AppVeyor, but it is triggered when
checking files out on Windows using recommended Git settings (Windows
newlines).

Workaround for
nikita-volkov/neat-interpolation#14, no more
necessary if that bug is fixed.
@nikita-volkov
Copy link
Owner

I don't quite understand the concern here. Am I correct that the preprocessor retains the same line -separators as were used in the source-code sent to the compiler?

Blaisorblade added a commit to commercialhaskell/stack that referenced this issue Aug 12, 2016
Note the issue is not triggered on AppVeyor, but it is triggered when
checking files out on Windows using recommended Git settings (Windows
newlines).

Workaround for
nikita-volkov/neat-interpolation#14, no more
necessary if that bug is fixed.

This defines a variant of shouldBe; while at it, also integrate needed
string type conversions.
@Blaisorblade
Copy link
Author

Blaisorblade commented Aug 12, 2016

Yes, you're correct.
I'm trying to explain why in many use cases that's not what you want. Or actually, I believe that's never what you want, but I anticipate that might be debatable.
EDIT: also, sorry for being unclear.

@Blaisorblade
Copy link
Author

Blaisorblade commented Aug 12, 2016

A better explanation. Do you agree that I should be able to replace "a\nb\n" with

[text|
a
b
|]

and get the same result, irrespective of the used newlines in the source file?

If so, your quasiquoter should modify newlines to be \n.

@nikita-volkov
Copy link
Owner

No problem. I completely disagree however.

I think you're projecting on this library the requirements, which one would expect from the editor (i.e., the automatic line-separator normalisation). All the imaginable code-editors out there have the related settings.

The library does only what it claims to do. If the user's code contains the \r\n character sequences it might just as well be by intent and I see no other unambiguous way to approach that. But, above all, I see no need in that, because, again, it's not of any library's concern to guess, which line endings the user intends to have.

@Blaisorblade
Copy link
Author

But, above all, I see no need in that, because, again, it's not of any library's concern to guess, which line endings the user intends to have.

My proposal would not pick Unix line endings for users; Windows line endings are converted to \n when opening a file in text mode, in all IO libraries around including Haskell (http://hackage.haskell.org/package/base-4.9.0.0/docs/GHC-IO-Handle-FD.html#v:openFile). This preserves the invariant that text strings in a program always use \n. Which is why putStr "A\nB\n" will use native endings on all platforms, and why lines and unlines have the same implementation on all platforms.

The truth is, this is probably a GHC bug.

Binary files are a separate matter, but that's not your usecase.

I think you're projecting on this library the requirements, which one would expect from the editor (i.e., the automatic line-separator normalisation). All the imaginable code-editors out there have the related settings.

Nope. I can't setup the editors of people checking out my sources, but by default (I guess) they'll preserve existing newlines. But again, Git won't.

Prior art

Other interpolation libraries work as I describe. Without even necessarily documenting it.

interpolatedstring-qq in Haskell:
http://hackage.haskell.org/package/interpolatedstring-qq-0.2/docs/src/Text-InterpolatedString-QQ.html#istr

Perl6 once violated the convention I describe, intentionally. Their entire testsuite became sensitive to users Git settings, and they changed their mind in 5 days after a ticket was filed.
https://rt.perl.org/Public/Bug/Display.html?id=126881
https://www.nu42.com/2015/12/perl6-newline-translation-broken.html
https://www.reddit.com/r/perl6/comments/3we6d6/newline_translation_in_perl6_is_broken/

@Blaisorblade
Copy link
Author

Here's a specific issue reaching the same conclusion:
23Skidoo/raw-strings-qq#1

@nikita-volkov
Copy link
Owner

Hmm. I'm honestly conflicted on this. On the one hand, you make arguments which I acknowledge and you're obviously informed on the issue's spread, on the other, this is a very ambiguous case.

I think the most "politically correct" way to approach this would be to introduce another quasi-quoter, which would provide the behaviour you expect. I however don't have time to approach this myself, I'm open to accept such a pull-request though.

@Blaisorblade
Copy link
Author

Thanks, I'll take a look at making a PR!

Blaisorblade added a commit to Blaisorblade/neat-interpolation that referenced this issue Aug 13, 2016
For nikita-volkov#14.

Missing:

- full testing.
- more complete docs.
- using textNL in examples?
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

2 participants