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

Support CRLF ("\r\n") as a separator for implicit splitting of output capture #970

Closed
xiaq opened this issue Apr 17, 2020 · 6 comments
Closed
Milestone

Comments

@xiaq
Copy link
Member

xiaq commented Apr 17, 2020

Currently:

~> @li = (echo "a\r\nb\r\n")
~> put $@li
▶ "a\r"
▶ "b\r"
▶ ''

With proposed change:

~> @li = (echo "a\r\nb\r\n")
~> put $@li
▶ "a"
▶ "b"

This is split from #918. Potential use case is making it easier to parse the output curl.

@xiaq
Copy link
Member Author

xiaq commented Apr 17, 2020

@SitiSchu I am not sure whether I want to support this: parsing the output of curl is a very specific use case, and you can already do that with

curl ... | splits "\r\n" (slurp)

@krader1961
Copy link
Contributor

I think this has less to do with supporting the output of curl and more to do with doing the right thing on MS Windows.

P.S., I despise MS Windows because of its numerous shortcomings. Not least of which is using \r\n for line endings solely because doing so meant you could cat a plain text file to a printer device and have it do the "right thing" more than four decades ago. Without any alteration of the text by the operating system. In other words, the \r\n was chosen as a line ending because each character in that pair would cause a dumb printer to do the "right thing" in terms of rendering the plain text.

@xiaq
Copy link
Member Author

xiaq commented Apr 23, 2020

You can find a lot of legacy stuff in UNIX that no longer makes sense today too :)

Also, I need to check that pipes in Windows actually use \r\n as line endings. With C on Windows, you have the distinction between "text mode" and "binary mode"; in text mode, you can pretend that the line ending is \n and stdio does the conversion for you. I am not sure if that applies to Go.

@xiaq xiaq changed the title Support CRLF ("\r\n") as a separator for implicit splitting Support CRLF ("\r\n") as a separator for implicit splitting of output capture May 3, 2020
@xiaq
Copy link
Member Author

xiaq commented May 3, 2020

Oh yeah, native Windows commands do use \r\n as the line ending when invoked from the console. In the spirit of doing the correct thing, Elvish should support \r\n as a separator for implicit splitting in output capture expressions.

This behavior should be adopted everywhere. This has the nice side effect of making it easier to parse the output of curl and (even more rarely) working with files with CRLF line endings on Unix, but as @krader1961 said this is more about doing the correct thing on Windows.

Code that really cares about the exact bytes can always use slurp to turn the entire byte output to a string in a lossless way.


Tangent to this issue:

Interestingly enough, a lone \n also works perfectly in the console (at least in a recent version of Windows 10); Go manages to get away with fmt.Println only writing a \n on Windows, which is perfectly fine when used interactively, and the only downside seems to be that if the output is saved in a file you get LF line endings instead of CRLF (golang/go#28822).

@xiaq xiaq added this to the 0.14 milestone May 3, 2020
@xiaq xiaq removed the maybe label May 3, 2020
@xiaq
Copy link
Member Author

xiaq commented May 3, 2020

This is a backward-incompatible change, but since it's so rare for Unix commands to write \r this change will be introduced directly.

@krader1961
Copy link
Contributor

Yeah, for plain text files the only time you're likely to see \r\n line endings on a UNIX platform is if the file a) originated on a Windows systems, or b) it's mandated by something like the HTTP standard. And when dealing with binary (non-plain text) files on UNIX you shouldn't be using or relying on functions that read "lines". So I agree that changing the behavior is low risk.

@xiaq xiaq closed this as completed in ba7bedc May 3, 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

2 participants