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

[10.x] Revert 17907: util: change inspect depth default #20089

Closed
wants to merge 2 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Apr 16, 2018

Same as #20017 but targeted specifically to v10.x-staging

Ping @addaleax @Trott @TimothyGu @cjihrig @joyeecheung

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

This reverts commit b994b8e.

This caused regressions in ecosystem code. While the change originally
was semver-major and could be postponed until after Node.js 10,
I think reverting it is a good choice at this point.

Also, I personally do not think defaulting to a shallow inspect
is a bad thing at all – quite the opposite: It makes `util.inspect()`
give an overview of an object, rather than providing a full
display of its contents. Changing the `depth` default to infinity
fundamentally changed the role that `util.inspect()` plays,
and makes output much more verbose and thus at times unusable
for `console.log()`-style debugging.

Fixes: #19405
Refs: #17907
@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 Apr 16, 2018
@jasnell jasnell modified the milestones: 11.0.0, 10.0.0 Apr 16, 2018
@jasnell
Copy link
Member Author

jasnell commented Apr 16, 2018

@jasnell jasnell changed the title Revert 17907: util: change inspect depth default [10.x] Revert 17907: util: change inspect depth default Apr 17, 2018
@jasnell jasnell added the v10.x label Apr 17, 2018
jasnell pushed a commit that referenced this pull request Apr 17, 2018
This reverts commit 8f15309.

PR-URL: #20089
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
jasnell pushed a commit that referenced this pull request Apr 17, 2018
This reverts commit b994b8e.

This caused regressions in ecosystem code. While the change originally
was semver-major and could be postponed until after Node.js 10,
I think reverting it is a good choice at this point.

Also, I personally do not think defaulting to a shallow inspect
is a bad thing at all – quite the opposite: It makes `util.inspect()`
give an overview of an object, rather than providing a full
display of its contents. Changing the `depth` default to infinity
fundamentally changed the role that `util.inspect()` plays,
and makes output much more verbose and thus at times unusable
for `console.log()`-style debugging.

Fixes: #19405
Refs: #17907

PR-URL: #20089
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
@jasnell
Copy link
Member Author

jasnell commented Apr 17, 2018

Landed in v10.x-staging in 47af0a0 and 554ed37

@jasnell jasnell closed this Apr 17, 2018
@TimothyGu TimothyGu deleted the revert-util-inspect-depth-10.x-staging branch April 18, 2018 15:19
@vsemozhetbyt

This comment has been minimized.

@richardlau
Copy link
Member

I can still reproduce the #19405 with https://nodejs.org/download/test/v10.0.0-test72549aa9cd/

Either the revert was not successful, or there is another cause.

72549aa looks like it's from master whereas this PR landed on v10.x-staging. The test builds for #19091 all appear to have a date in the directory name, e.g., https://nodejs.org/download/test/v10.0.0-test20180417fa00e9cc5b/

@vsemozhetbyt
Copy link
Contributor

@richardlau Yes, sorry, I've tested with https://nodejs.org/download/rc/v10.0.0-rc.1/ and it is OK.

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