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.

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

Revert "repl,util: insert carriage returns in output" #8141

Closed
wants to merge 1 commit into from

Conversation

evanlucas
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

repl,util

Description of change

This reverts commit fce4b98.

This was a breaking change and should have been marked semver-major.

Ref: #8028
Related: hapijs/code#81
Related: #8138

This reverts commit fce4b98.

This was a breaking change and should have been marked semver-major.
@nodejs-github-bot nodejs-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 17, 2016
@Fishrock123
Copy link
Contributor

LGTM, I was kinda worried about it.

@Fishrock123
Copy link
Contributor

Marking the original PR as major.

@yorkie
Copy link
Contributor

yorkie commented Aug 17, 2016

LGTM and I think we should also mention @maxrimue, who opened #8138, to know this reverting.

@addaleax
Copy link
Member

LGTM and I am still a bit uncomfortable about how there was never any actual reason for the change provided in the first place, other than “it fixed something in one of my projects”.

@evanlucas evanlucas added the v6.x label Aug 17, 2016
@evanlucas evanlucas changed the base branch from v6.x to master August 17, 2016 14:52
@evanlucas evanlucas changed the base branch from master to v6.x August 17, 2016 14:52
@maxrimue
Copy link

This is great, I rather consider this a major change as well. Thanks everyone!

@jasnell
Copy link
Member

jasnell commented Aug 17, 2016

What did this break? Why does it need to be reverted? The PR can be marked semver-major after it landed.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 17, 2016

@jasnell it breaks anything that relies on output only containing \n, but now contains the additional \r. See hapijs/code#82 for example.

@addaleax
Copy link
Member

Also, the tests for yargs broke (#8138) and the PR (#8028) has been labelled as semver-major today.

@evanlucas
Copy link
Contributor Author

@jasnell look at the two related links in the OP for what it broke.

This changed the output of util.format()

@jasnell
Copy link
Member

jasnell commented Aug 17, 2016

Ok, so it did land in v6 .. that's unfortunate. Wouldn't we only revert it in v6 then? Or is this enough of a break that we don't want it in master either?

@evanlucas
Copy link
Contributor Author

I opened the PR on master because I agree with @addaleax's comment:

LGTM and I am still a bit uncomfortable about how there was never any actual reason for the change provided in the first place, other than “it fixed something in one of my projects”.

I feel like we need better justification to a pretty large breaking change like this

@jasnell
Copy link
Member

jasnell commented Aug 17, 2016

Works for me. Would you mind adding a bit more of that justification to the commit message?

@mscdex
Copy link
Contributor

mscdex commented Aug 17, 2016

FWIW I didn't recall seeing util.format() changed in the original PR, the intention was to only change repl.js.

@addaleax
Copy link
Member

I would have read #7954 (comment) as a request to change it for util.inspect/util.format, too.

@evanlucas
Copy link
Contributor Author

Closing this one as #8143 is open for master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants