Skip to content

repl,util: insert carriage returns in output#8028

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
Merged
JungMinu merged 1 commit into from
Aug 13, 2016
Merged

repl,util: insert carriage returns in output #8028

JungMinu merged 1 commit into from
Aug 13, 2016

Conversation

@JungMinu
Copy link
Member

@JungMinuJungMinu commented Aug 9, 2016•
edited
Loading

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

repl, util

Description of change

\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

@nodejs-github-botnodejs-github-bot added repl Issues and PRs related to the REPL subsystem. util Issues and PRs related to the built-in util module. labels Aug 9, 2016
@JungMinuJungMinu self-assigned this Aug 9, 2016
@JungMinu
Copy link
MemberAuthor

JungMinu commented Aug 9, 2016•
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/3586/
/cc @mscdex@jasnell@Fishrock123@addaleax@julianduque

@addaleax
Copy link
Member

addaleax commented Aug 10, 2016

I guess this looks okay (and semver-major-y?) but tbh I didn’t really understand the problem it is supposed to solve… usually TTYs should have their ONLCR flag set by default, translating \n to \r\n?

@mscdex
Copy link
Contributor

mscdex commented Aug 10, 2016

@addaleax I'm not sure why it was happening that way, but that is the problem I was running into with a module I wrote.

@JungMinu
Copy link
MemberAuthor

JungMinu commented Aug 11, 2016

@mscdex PTAL, LGTY?

@mscdex
Copy link
Contributor

mscdex commented Aug 11, 2016

There are still various places that still only use \n, especially anything that is written to the outputStream.

@JungMinu
Copy link
MemberAuthor

JungMinu commented Aug 12, 2016

@mscdex Done, PTAL again :)

@JungMinu
Copy link
MemberAuthor

JungMinu commented Aug 12, 2016

@mscdex If LGTY, I will run a new CI

@Fishrock123
Copy link
Contributor

Fishrock123 commented Aug 12, 2016

Would it bet better to use os.EOL for this?

@addaleax
Copy link
Member

addaleax commented Aug 12, 2016

See #7954 (comment) … so, apparently no, but I’d still really like to understand what the real problem here is that this is trying to solve.

@JungMinu
Copy link
MemberAuthor

JungMinu commented Aug 13, 2016

gentle ping @mscdex

@mscdex
Copy link
Contributor

mscdex commented Aug 13, 2016

I think that should do it. LGTM if CI is ok with it.

@JungMinu
Copy link
MemberAuthor

JungMinu commented Aug 13, 2016

new CI: https://ci.nodejs.org/job/node-test-pull-request/3652/

@JungMinu
Copy link
MemberAuthor

JungMinu commented Aug 13, 2016

CI is green :)

@JungMinu
repl,util: insert carriage returns in output
fce4b98
`\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]>
@JungMinuJungMinu merged commit fce4b98 into nodejs:masterAug 13, 2016
@JungMinu
Copy link
MemberAuthor

JungMinu commented Aug 13, 2016

Landed in fce4b98

@JungMinuJungMinu mentioned this pull request Aug 13, 2016
Closed
cjihrig pushed a commit that referenced this pull request 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]>
This was referenced Aug 15, 2016
Merged
Closed
@evanlucas
Copy link
Contributor

evanlucas commented Aug 17, 2016

I feel like this should have been marked as a breaking change.

@yorkie
Copy link
Contributor

yorkie commented Aug 17, 2016

+1 on marking as breaking change.

@evanlucasevanlucas mentioned this pull request Aug 17, 2016
Closed
2 tasks
@Fishrock123Fishrock123 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 17, 2016
@evanlucasevanlucas mentioned this pull request Aug 17, 2016
Merged
2 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@JungMinuJungMinu

Labels

replIssues and PRs related to the REPL subsystem.semver-majorPRs that contain breaking changes and should be released in the next major version.utilIssues and PRs related to the built-in util module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

7 participants

@JungMinu@addaleax@mscdex@Fishrock123@evanlucas@yorkie@nodejs-github-bot