Skip to content
This repository was archived by the owner on Jul 17, 2018. It is now read-only.

Comments

Add support for Windows newlines in the log#175

Open
zdavidsen wants to merge 1 commit intomeepen:masterfrom
zdavidsen:windows-newlines
Open

Add support for Windows newlines in the log#175
zdavidsen wants to merge 1 commit intomeepen:masterfrom
zdavidsen:windows-newlines

Conversation

@zdavidsen
Copy link

Really not an issue for most text editors, but Notepad requires CRLF and it's a quick easy fix

@meepen
Copy link
Owner

meepen commented Jul 2, 2018

Usually the mode you open a file handles these things for you, this is probably the wrong way

@zdavidsen
Copy link
Author

Hmm, I'll take a look and see if I can't find a better way

@zdavidsen
Copy link
Author

It's worth mentioning that pretty much the only program that this affects is Notepad, as pretty much every other program handles LF gracefully. Though Notepad is the default, I imagine that most people who are actually checking the log probably have another editor on hand. Another detail pointed out in this thread is that different line endings could break parsing, though I don't imagine that should be a huge issue here?

That said, I can't find any node documentation that suggests that line endings can be auto handled based on the file mode. The only solution I've found exposed is os.EOL, which involves another require, but would be a better solution.

Apologies if I've missed something better, I've never worked directly with node.js before, so this is mostly pulled from some quick searches and skimming the docs.

@meepen
Copy link
Owner

meepen commented Jul 3, 2018 via email

@zdavidsen
Copy link
Author

It's a little hard to tell, but it looks like node already writes in non-binary if you pass the write function a string, and only writes in binary if you pass it a buffer? semi relevant doc

I'll switch it over to use os tomorrow when I get some time. Would you prefer to keep os in a const global like fs and then just drop os.EOL in for the newline character, or just pull os.EOL into a let and not keep all of os around?

@meepen
Copy link
Owner

meepen commented Jul 4, 2018

const os = require("os"); should be fine

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants