Skip to content

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-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 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

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”.

@evanlucasevanlucas changed the base branch from v6.x to masterAugust 17, 2016 14:52
@evanlucasevanlucas changed the base branch from master to v6.xAugust 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

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

@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
ContributorAuthor

@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

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
ContributorAuthor

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

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

@mscdex
Copy link
Contributor

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
ContributorAuthor

Closing this one as #8143 is open for master.

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

Labels

replIssues and PRs related to the REPL subsystem.utilIssues and PRs related to the built-in util module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@evanlucas@Fishrock123@yorkie@addaleax@maxrimue@jasnell@cjihrig@mscdex@nodejs-github-bot