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

util: use constructor name by BridgeAR · Pull Request #14886 · nodejs/node · GitHub

/ node Public

util: use constructor name - #14886

Closed
BridgeAR wants to merge 1 commit into
nodejs:masterfrom
BridgeAR:add-proper-constructor
ConversationCommits (1)ChecksFiles changed
Closed

util: use constructor name#14886
BridgeAR wants to merge 1 commit into
nodejs:masterfrom
BridgeAR:add-proper-constructor

Conversation

BridgeAR commented Aug 17, 2017

Copy link
Copy Markdown
Member

For circular references util.inspect always prints Array
or Object not matter if it is a subclass or not.

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
Affected core subsystem(s)

util

nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Aug 17, 2017
BridgeAR commented Aug 17, 2017
View reviewed changes
Comment thread lib/util.js Outdated

BridgeAR Aug 17, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Note: this check is redundant as soon as #14881 has landed. Right now Arrays set the constructor to null and the check has to be done again.

BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 17, 2017

TimothyGu commented Aug 17, 2017

Copy link
Copy Markdown
Member

Commit message nit: doesn't this apply to objects that exceed the depth limit rather than circular objects? Or both?

BridgeAR force-pushed the add-proper-constructor branch from d43d928 to d76998e Compare August 17, 2017 17:08

BridgeAR commented Aug 17, 2017

Copy link
Copy Markdown
Member Author

@TimothyGu you are absolutely right! Fixed

BridgeAR commented Aug 23, 2017

Copy link
Copy Markdown
Member Author

PTAL I changed the commit message and this could land otherwise.

hiroppy commented Aug 26, 2017

Copy link
Copy Markdown
Member

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

BridgeAR commented Aug 26, 2017

Copy link
Copy Markdown
Member Author

I would like to land this after #14881 as that one would otherwise not cleanly land on 8.x. Otherwise I would have already landed that. Marking this as blocked because of that.

BridgeAR added the blocked PRs that are blocked by other issues or PRs. label Aug 26, 2017
BridgeAR removed the blocked PRs that are blocked by other issues or PRs. label Sep 15, 2017
util: use constructor name
572f1ef 
When reaching the depth limit util.inspect always prints [Array]
or [Object] no matter if it is a subclass or not.
This fixes it by showing the actual constructor name instead.
BridgeAR force-pushed the add-proper-constructor branch from d76998e to 572f1ef Compare September 15, 2017 01:42

BridgeAR commented Sep 15, 2017

Copy link
Copy Markdown
Member Author

Rebased. PTAL, I would like to land this very soon.

BridgeAR commented Sep 19, 2017

Copy link
Copy Markdown
Member Author

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

BridgeAR commented Sep 19, 2017

Copy link
Copy Markdown
Member Author

Landed in b1c8f15

BridgeAR closed this Sep 19, 2017
BridgeAR added a commit that referenced this pull request Sep 19, 2017
util: use constructor name
b1c8f15 
When reaching the depth limit util.inspect always prints [Array]
or [Object] no matter if it is a subclass or not.
This fixes it by showing the actual constructor name instead.

PR-URL: #14886
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
util: use constructor name
e1e9eb1 
When reaching the depth limit util.inspect always prints [Array]
or [Object] no matter if it is a subclass or not.
This fixes it by showing the actual constructor name instead.

PR-URL: nodejs/node#14886
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
util: use constructor name
f2844dc 
When reaching the depth limit util.inspect always prints [Array]
or [Object] no matter if it is a subclass or not.
This fixes it by showing the actual constructor name instead.

PR-URL: nodejs/node#14886
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
jasnell mentioned this pull request Oct 30, 2017
Merged
BridgeAR deleted the add-proper-constructor branch April 1, 2019 23:37
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

jasnell jasnell approved these changes
cjihrig cjihrig approved these changes
+3 more reviewers
refack refack approved these changes
TimothyGu TimothyGu approved these changes
hiroppy hiroppy approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

semver-major PRs that contain breaking changes and should be released in the next major version. 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.

7 participants

Back | FazBrowse Home | New Git URL