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.

Sign up for GitHub

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

Jump to bottom

Newlines missing carriage returns in REPL, util.inspect() output #7954

Closed
mscdex opened this issue Aug 3, 2016 · 7 comments · Fixed by #8028
Closed

Newlines missing carriage returns in REPL, util.inspect() output #7954

mscdex opened this issue Aug 3, 2016 · 7 comments · Fixed by #8028
Assignees
@JungMinu
Labels
good first issue Issues that are suitable for first-time contributors. repl Issues and PRs related to the REPL subsystem. util Issues and PRs related to the built-in util module.

Comments

@mscdex
Copy link
Contributor

mscdex commented Aug 3, 2016 •
edited
Loading

  • Version: v6.3.1
  • Platform: Linux
  • Subsystem: repl, util

I noticed that the repl module and util.inspect() only write \n and not \r\n in their output. readline however consistently writes \r\n to its output.

This seemingly causes output problems with custom REPLs with custom output streams. I'm not exactly sure why this is a problem though, since the output streams eventually reach a (Linux) terminal. The current workaround I have is to create a newline conversion stream (that inserts carriage returns) that then pipes to my actual output stream. However, I'd prefer not to have to use this workaround forever.

This should just be a matter of changing the relevant instances of \n to \r\n in lib/repl.js and lib/util.js.
@mscdex mscdex added util Issues and PRs related to the built-in util module. repl Issues and PRs related to the REPL subsystem. good first issue Issues that are suitable for first-time contributors. labels Aug 3, 2016
@imyller
Copy link
Member

imyller commented Aug 3, 2016

Userland is encouraged to use os.EOL instead of hardcoded \n or \r\n for improved portability.

Why not use that in Node.js core libraries? console, repl, util etc.

@mscdex
Copy link
Contributor Author

mscdex commented Aug 3, 2016 •
edited
Loading

Well as I said, for some reason \n is not enough for Linux with my custom stream (so os.EOL would not help), I'm not sure why and I haven't really dug into the issue to find out why. Local REPLs work just fine though.

AFAIK using \r\n everywhere should not be a problem, even on non-Windows platforms, and it ensures that the output is displayed correctly.

@JungMinu JungMinu self-assigned this Aug 3, 2016
@JungMinu
Copy link
Member

JungMinu commented Aug 3, 2016

I will resolve this issue :)

@imyller
Copy link
Member

imyller commented Aug 3, 2016

I'm not sure why and I haven't really dug into the issue to find out why.

So, changing core lib to use \r\n masks the real issue. Maybe test case could be created?

AFAIK using \r\n everywhere should not be a problem, even on non-Windows platforms, and it ensures that the output is displayed correctly.

I agree with that. I use \r\n universally just to be sure.

@mscdex
Copy link
Contributor Author

mscdex commented Aug 3, 2016

I'm not sure how you could create a test case to check for visual layout. I think about the best you could do is just to have a test that checks that only \r\n exists in the output and not just \n.

@JungMinu
Copy link
Member

JungMinu commented Aug 4, 2016

Yep, I will create a pull request ASAP 😄

@JungMinu JungMinu mentioned this issue Aug 9, 2016
Merged
3 tasks
@JungMinu
Copy link
Member

#8028 has now landed in fce4b98

cjihrig pushed a commit that referenced this issue Aug 15, 2016
@JungMinu @cjihrig
repl,util: insert carriage returns in output
08e6eee 
`\n` is not enough for Linux with some custom stream
add carriage returns to ensure that the output is displayed correctly
using `\r\n` should not be a problem, even on non-Windows platforms.

Fixes: #7954
PR-URL: #8028
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@addaleax addaleax mentioned this issue Aug 17, 2016
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@JungMinu JungMinu

Labels
good first issue Issues that are suitable for first-time contributors. repl Issues and PRs related to the REPL subsystem. util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

repl,util: insert carriage returns in output
3 participants
@mscdex @imyller @JungMinu