util: remove custom inspection function#20722

BridgeAR · 2018-05-14T16:57:18Z

This removes the deprecated custom inspection function and fixes
all tests accordingly.

Refs: #15549

I guess we could merge #20525 first and land this one right afterwards.

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 removes the deprecated custom inspection function and fixes all tests accordingly. Refs: nodejs#15549

BridgeAR · 2018-05-14T16:59:08Z

CI https://ci.nodejs.org/job/node-test-pull-request/14867/ ✔️

mscdex · 2018-05-14T17:24:23Z

test/parallel/test-console.js


-// an Object with a custom .inspect() function
-constcustom_inspect={foo: 'bar',inspect: ()=>'inspect'};
+// An Object with a custom .inspect() function.


Perhaps this should be slightly re-worded to instead say "custom inspect function" or "custom inspect symbol" or similar now, to avoid confusion.

I would say it is still a custom inspect function but it might be best to just write .inspect() without dot and brackets.

mscdex · 2018-05-14T17:26:34Z

test/parallel/test-util-inspect.js


 util.inspect(process);

 // Setting custom inspect property to a non-function should do nothing.


s/property/symbol/ ?

But it is still a property. It is just accessed with a symbol.

BridgeAR · 2018-05-15T11:21:03Z

I think it is fine to not run the CI again for the changed comment.

mcollina

LGTM

BridgeAR · 2018-05-17T15:07:15Z

I forgot to remove the docs. I just fixed that.

addaleax · 2018-05-17T15:11:29Z

doc/api/util.md

-`util.inspect()` will invoke and use the result of when inspecting the object:
+Objects may also define their own `[util.inspect.custom](depth, opts)` function
+that `util.inspect()` will invoke and use the result of when inspecting the
+object:


There’s also the customInspect option description which references inspect(){… }, fwiw

BridgeAR · 2018-05-18T14:00:52Z

@vsemozhetbyt would you be so kind and have a brief look at the changed docs before landing? :-)

BridgeAR · 2018-05-18T14:01:10Z

Or @Trott

vsemozhetbyt

Docs LGTM with possible nits)

vsemozhetbyt · 2018-05-18T15:15:36Z

doc/api/util.md

-*`customInspect`{boolean} If `false`, then custom `inspect(depth, opts)`
- functions will not be called. **Default:**`true`.
+*`customInspect`{boolean} If `false`, then
+`[util.inspect.custom](depth, opts)` functions will not be called.


Can many functions be called during the inspection? Does this mean nested objects with own custom functions?

You are right, it is the same function (if it does not replace itself during custom inspection ;-) ).

Looking at this again: functions do seem to be correct as it is not about a specific custom inspect function but about custom inspect functions in general.

vsemozhetbyt · 2018-05-18T15:26:57Z

doc/api/util.md

 `util.inspect()` will use the constructor's name and/or `@@toStringTag` to make
 an identifiable tag for an inspected value.

+Values may supply their own custom inspection function


As the next code example does not contain any mention of this, maybe we should link inspection function here to #util_util_inspect_custom or #util_custom_inspection_functions_on_objects?

Yes, I was also wrapping my head around this. I moved it because when looking at the current documentation it is confusing as it seems to be about something else than it really is. So +1 on that.

vsemozhetbyt · 2018-05-18T15:27:15Z

doc/api/util.md

 an identifiable tag for an inspected value.

+Values may supply their own custom inspection function
+(`[util.inspect.custom](depth, opts)`), when called these receive the current


function -> functions or these receive -> this receives?

vsemozhetbyt · 2018-05-18T15:27:38Z

doc/api/util.md


+Values may supply their own custom inspection function
+(`[util.inspect.custom](depth, opts)`), when called these receive the current
+`depth` in the recursive inspection, as well as the options object passed to


options -> `options`?

This is already sufficiently covered in the docs.

BridgeAR · 2018-05-19T16:14:54Z

Comments addressed (I actually removed the whole paragraph as it is already sufficiently covered in the docs and the current docs are more confusing than helpful).

PTAL

I would like to land this some time soon.

vsemozhetbyt · 2018-05-19T17:12:32Z

Doc changes LGTM)

BridgeAR · 2018-05-19T17:30:45Z

Thanks.

Landed in 27df81c 🎉

This removes the deprecated custom inspection function and fixes all tests accordingly. Refs: nodejs#15549 PR-URL: nodejs#20722 Refs: nodejs#15549 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

Trott · 2018-05-20T03:38:13Z

semver-major but no citgm run?

BridgeAR · 2018-05-21T02:53:59Z

@Trott sorry, I missed that. However, later CITGM runs did not show up any result. I did find usage of it though and I am going to fix those in the next days.

Ports the `buffer-inspect` test from Node core. Adds a `Buffer.prototype[util.inspect.custom]` method, which is an alias to `Buffer.prototype.inspect`. Node already doesn't have an `.inspect` method on Buffers anymore, but since this module has to work in browsers that do not have Symbols, it seems better to keep it around. In Node, this will use the builtin `util.inspect.custom` symbol. In the browser, it will use `Symbol.for('util.inspect.custom')`. The browser version of `util` will also use the `inspect-custom-symbol` module in the near future. If nodejs/node#20857 gets merged, `Symbol.for('util.inspect.custom')` will be used everywhere and the dependency on `inspect-custom-symbol` could probably be dropped. The motivation for this is API parity and the fact that Node is removing support for the old `.inspect` method: nodejs/node#20722

util: remove custom inspection function
50c95a9
This removes the deprecated custom inspection function and fixes all tests accordingly. Refs: nodejs#15549

BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 14, 2018

BridgeAR requested review from a team and Trott May 14, 2018 16:57

nodejs-github-bot added the util Issues and PRs related to the built-in util module. label May 14, 2018

addaleax approved these changes May 14, 2018
View reviewed changes

BridgeAR mentioned this pull request May 14, 2018
util: move custom inspection function to EOL #20525
Closed
2 tasks

mscdex reviewed May 14, 2018
View reviewed changes

fixup: address comment
79d7790

BridgeAR requested a review from a teamMay 15, 2018 11:20

mcollina approved these changes May 15, 2018
View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 17, 2018

BridgeAR requested a review from a teamMay 17, 2018 15:04

BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 17, 2018

fixup: remove docs
2906612

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 17, 2018

addaleax reviewed May 17, 2018
View reviewed changes

jasnell approved these changes May 17, 2018
View reviewed changes

fixup: address comment
5e9d5d5

vsemozhetbyt reviewed May 18, 2018
View reviewed changes

fixup: remove unnecessary docs part
ca82165
This is already sufficiently covered in the docs.

BridgeAR closed this May 19, 2018

BridgeAR added the notable-change PRs with changes that should be highlighted in changelogs. label May 19, 2018

BridgeAR mentioned this pull request May 28, 2018
Deprecate object.inspect for custom inspection #15549
Closed

jdalton mentioned this pull request Jun 5, 2018
Custom inspection warning #21119
Closed

goto-bus-stop mentioned this pull request Jun 20, 2018
Use util.inspect.custom symbol if available. feross/buffer#204
Merged

targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 6, 2018

BridgeAR deleted the remove-custom-inspect branch January 20, 2020 11:33


		util.inspect(process);

		// Setting custom inspect property to a non-function should do nothing.

Uh oh!

util: remove custom inspection function#20722

util: remove custom inspection function #20722

Uh oh!

Conversation

BridgeAR commented May 14, 2018

Checklist

Uh oh!

BridgeAR commented May 14, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdexMay 14, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented May 15, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented May 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented May 18, 2018

Uh oh!

BridgeAR commented May 18, 2018

Uh oh!

vsemozhetbyt left a comment

Choose a reason for hiding this comment

Uh oh!

vsemozhetbytMay 18, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented May 19, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vsemozhetbyt commented May 19, 2018

Uh oh!

BridgeAR commented May 19, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented May 20, 2018

Uh oh!

BridgeAR commented May 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

BridgeAR commented May 14, 2018•
edited
Loading

mscdexMay 14, 2018•
edited
Loading

vsemozhetbytMay 18, 2018•
edited
Loading

BridgeAR commented May 19, 2018•
edited
Loading

BridgeAR commented May 19, 2018•
edited
Loading