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

Add support for Windows newlines in the log #175

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add support for Windows newlines in the log #175

wants to merge 1 commit into from

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