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

Use CRLF on Windows #326

Closed
tniessen opened this Issue Nov 2, 2017 · 3 comments

Comments

Projects
None yet
3 participants
@tniessen
Contributor

tniessen commented Nov 2, 2017

pino seems to always emit \n, but it should at least be configurable to emit \r\n on Windows, or even make it a default.

This seems to be caused by

pino/pino.js

Line 277 in ce10ba1

iopts.end = ',"v":' + LOG_VERSION + '}\n'

and by multiple other occurrences in https://github.com/pinojs/pino/blob/master/pretty.js.

@jsumners

This comment has been minimized.

Show comment
Hide comment
@jsumners

jsumners Nov 2, 2017

Collaborator

Hmm, I was prepared to shoot this down, but it looks like the spec allows for \r\n:

http://specs.okfnlabs.org/ndjson/

Each JSON text MUST conform to the [RFC7159] standard and MUST be written to the stream followed by the newline character \n (0x0A). The newline charater MAY be preceeded by a carriage return \r (0x0D). The JSON texts MUST NOT contain newlines or carriage returns.

Would you like to submit a PR to enable this option?

Collaborator

jsumners commented Nov 2, 2017

Hmm, I was prepared to shoot this down, but it looks like the spec allows for \r\n:

http://specs.okfnlabs.org/ndjson/

Each JSON text MUST conform to the [RFC7159] standard and MUST be written to the stream followed by the newline character \n (0x0A). The newline charater MAY be preceeded by a carriage return \r (0x0D). The JSON texts MUST NOT contain newlines or carriage returns.

Would you like to submit a PR to enable this option?

@davidmarkclements

This comment has been minimized.

Show comment
Hide comment
@davidmarkclements

davidmarkclements Nov 2, 2017

Member

any tools built around pino at this point may be relying on just \n - in there interest of not breaking ecosystem libs let's make this an opt-in setting

Member

davidmarkclements commented Nov 2, 2017

any tools built around pino at this point may be relying on just \n - in there interest of not breaking ecosystem libs let's make this an opt-in setting

@tniessen

This comment has been minimized.

Show comment
Hide comment
@tniessen

tniessen Nov 2, 2017

Contributor

@davidmarkclements Yes, #327 makes it completely optional. I would actually prefer to always use \n but some Windows tools were designed for \r\n.

Contributor

tniessen commented Nov 2, 2017

@davidmarkclements Yes, #327 makes it completely optional. I would actually prefer to always use \n but some Windows tools were designed for \r\n.

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