FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Revert "util: change util.inspect depth default" by devsnek · Pull Request #24326 · nodejs/node · GitHub

/ node Public

Revert "util: change util.inspect depth default" - #24326

Closed
devsnek wants to merge 1 commit into
nodejs:masterfrom
devsnek:revert/ac7450a09a4c167cd43c14d7c88721d22f077529
ConversationCommits (1)ChecksFiles changed
Closed

Revert "util: change util.inspect depth default"#24326
devsnek wants to merge 1 commit into
nodejs:masterfrom
devsnek:revert/ac7450a09a4c167cd43c14d7c88721d22f077529

Conversation

devsnek commented Nov 12, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

This reverts commit ac7450a.

It has caused breakage in logging to existing apps, and even
something as simple as console.log(require) will cause >1m freezes.

I've heard nothing but negative feedback (seriously not a single
person has expressed anything positive about this change) and
personally i find this change extremely annoying while debugging.

cc @nodejs/util

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

devsnek requested a review from BridgeAR November 12, 2018 21:29

This comment has been minimized.

Sign in to view
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 Nov 12, 2018
Revert "util: change util.inspect depth default"
bde132b 
This reverts commit ac7450a.

This fully reverts the changes to util.inspect depth.

It has caused breakage in logging to existing apps, and even
something as simple as `console.log(require)` will cause >1m freezes.

I've heard nothing but negative feedback (seriously not a single
person has expressed anything positive about this change) and
personally i find this change extremely annoying.
devsnek force-pushed the revert/ac7450a09a4c167cd43c14d7c88721d22f077529 branch from 4ef10a3 to bde132b Compare November 12, 2018 21:53
devsnek changed the title revert c7450a, 1a1fe53 Revert "util: change util.inspect depth default" Nov 12, 2018

devsnek commented Nov 12, 2018

Copy link
Copy Markdown
Member Author

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

devsnek added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Nov 12, 2018

BridgeAR commented Nov 13, 2018

Copy link
Copy Markdown
Member

I was not aware that #23350 never landed. Shall we go for that instead or shall we revert everything fully first? I am fine either way as I intend to do a breadth wide inspection instead of the current inspection which allows to check for the actual lines being printed and therefore have a reasonable way to limit the output. The implementation is just not that trivial.

devsnek commented Nov 13, 2018

Copy link
Copy Markdown
Member Author

@BridgeAR I'd prefer it go all the way back to 2, and then we can work our way up if that is deemed the best direction.

Trott commented Nov 15, 2018

Copy link
Copy Markdown
Member

Landed in 7082c61

Trott closed this Nov 15, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 15, 2018
Revert "util: change util.inspect depth default"
7082c61 
This reverts commit ac7450a.

This fully reverts the changes to util.inspect depth.

It has caused breakage in logging to existing apps, and even
something as simple as `console.log(require)` will cause >1m freezes.

I've heard nothing but negative feedback (seriously not a single
person has expressed anything positive about this change) and
personally i find this change extremely annoying.

PR-URL: nodejs#24326
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
devsnek deleted the revert/ac7450a09a4c167cd43c14d7c88721d22f077529 branch November 15, 2018 04:16
devsnek removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Nov 15, 2018
BridgeAR mentioned this pull request Nov 15, 2018
Merged
BridgeAR added the notable-change PRs with changes that should be highlighted in changelogs. label Nov 15, 2018

BridgeAR commented Nov 15, 2018

Copy link
Copy Markdown
Member

It would have been good to include the other commit from the original PR as well. Otherwise the default depth for the %o and %O formatter falls back to 2 instead of 4 as before.

devsnek commented Nov 15, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

@BridgeAR I left that commit out on purpose, as it wasn't breaking anything and seems reasonable to not override anything.

devsnek commented Nov 15, 2018

Copy link
Copy Markdown
Member Author

@BridgeAR then revert it i guess? regardless of that, this revert was approved and merged and should be in a release

BridgeAR commented Nov 15, 2018

Copy link
Copy Markdown
Member

It was my intention to release it but it broke tests without the other commit backported as well. That's why I pulled it out again as I did not want to block the release. So there is no disagreement here.

targos commented Nov 18, 2018

Copy link
Copy Markdown
Member

What's the next step here? Open a backport PR for v11.x that includes a revert of the other commit?

devsnek commented Nov 18, 2018

Copy link
Copy Markdown
Member Author

i'm not really sure. apparently some tests are failing on 11.x that aren't failing on master 🤷‍♂️

richardlau mentioned this pull request Dec 5, 2018
Closed
4 tasks
BridgeAR added a commit that referenced this pull request Dec 5, 2018
2018-12-06, Version 11.4.0 (Current), @BridgeAR
32ee780 
Notable Changes:

* util:
  The inspection `depth` default is now back at 2.
  #24326
* Added new collaborator:
  [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
  #24655
BridgeAR mentioned this pull request Dec 5, 2018
Merged
4 tasks
BridgeAR added a commit that referenced this pull request Dec 5, 2018
2018-12-06, Version 11.4.0 (Current), @BridgeAR
b0273f1 
Notable Changes:

* util:
  The inspection `depth` default is now back at 2.
  #24326
* Added new collaborator:
  [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
  #24655
BridgeAR added a commit that referenced this pull request Dec 6, 2018
2018-12-06, Version 11.4.0 (Current), @BridgeAR
2e6e799 
Notable Changes:

* util:
  The inspection `depth` default is now back at 2.
  #24326
* Added new collaborator:
  [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
  #24655

PR-URL: #24854
BridgeAR added a commit that referenced this pull request Dec 6, 2018
2018-12-06, Version 11.4.0 (Current)
758cb07 
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    #23708
  * The inspection `depth` default is now back at 2.
    #24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    #23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    #24739
* readline:
  * The `readline` module now supports async iterators.
    #23916
* repl:
  * The multiline history feature is removed.
    #24804
* tls:
  * Added min/max protocol version options.
    #24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. #24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    #23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    #24677
  * The install-tools scripts or now included in the dist.
    #24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    #24655

PR-URL: #24854
BridgeAR added a commit that referenced this pull request Dec 7, 2018
2018-12-07, Version 11.4.0 (Current)
4f1297f 
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    #23708
  * The inspection `depth` default is now back at 2.
    #24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    #23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    #24739
* readline:
  * The `readline` module now supports async iterators.
    #23916
* repl:
  * The multiline history feature is removed.
    #24804
* tls:
  * Added min/max protocol version options.
    #24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. #24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    #23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    #24677
  * The install-tools scripts or now included in the dist.
    #24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    #24655

PR-URL: #24854
BridgeAR added a commit that referenced this pull request Dec 7, 2018
2018-12-07, Version 11.4.0 (Current)
bcef949 
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    #23708
  * The inspection `depth` default is now back at 2.
    #24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    #23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    #24739
* readline:
  * The `readline` module now supports async iterators.
    #23916
* repl:
  * The multiline history feature is removed.
    #24804
* tls:
  * Added min/max protocol version options.
    #24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. #24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    #23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    #24677
  * The install-tools scripts or now included in the dist.
    #24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    #24655

PR-URL: #24854
This was referenced Dec 7, 2018
Closed
Closed

kapouer commented Dec 7, 2018

Copy link
Copy Markdown
Contributor

Did anyone actually checked the claim that util.inspect(require, {depth: 20}) took one minute ?

This was referenced Dec 7, 2018
Closed
Closed
Closed
yous mentioned this pull request Jan 7, 2019
Merged
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
2018-12-07, Version 11.4.0 (Current)
5912f6d 
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    nodejs#23708
  * The inspection `depth` default is now back at 2.
    nodejs#24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    nodejs#23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    nodejs#24739
* readline:
  * The `readline` module now supports async iterators.
    nodejs#23916
* repl:
  * The multiline history feature is removed.
    nodejs#24804
* tls:
  * Added min/max protocol version options.
    nodejs#24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. nodejs#24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    nodejs#23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    nodejs#24677
  * The install-tools scripts or now included in the dist.
    nodejs#24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    nodejs#24655

PR-URL: nodejs#24854
inoway46 mentioned this pull request Aug 16, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

addaleax addaleax approved these changes
cjihrig cjihrig approved these changes
BridgeAR BridgeAR approved these changes
+3 more reviewers
jdalton jdalton approved these changes
fhinkel fhinkel approved these changes
targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

notable-change PRs with changes that should be highlighted in changelogs. 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 this pull request may close these issues.

10 participants

Back | FazBrowse Home | New Git URL