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

@@ -46,8 +47,8 @@ assert.throws(() => console.timeEnd(Symbol('test')),
              TypeError);


-// an Object with a custom .inspect() function
-const custom_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

@@ -1162,8 +1110,11 @@ 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

@@ -426,6 +426,11 @@ passed that alter certain aspects of the formatted string.
 `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

@@ -426,6 +426,11 @@ passed that alter certain aspects of the formatted string.
 `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
+(`[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

@@ -426,6 +426,11 @@ passed that alter certain aspects of the formatted string.
 `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
+(`[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 <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 Trott and a team 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 team May 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 team May 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util: remove custom inspection function #20722

util: remove custom inspection function #20722

BridgeAR commented May 14, 2018

BridgeAR commented May 14, 2018 •

edited

Loading

mscdex May 14, 2018 •

edited

Loading

BridgeAR May 15, 2018

mscdex May 14, 2018

BridgeAR May 15, 2018

BridgeAR commented May 15, 2018

mcollina left a comment

BridgeAR commented May 17, 2018

addaleax May 17, 2018

BridgeAR May 17, 2018

BridgeAR commented May 18, 2018

BridgeAR commented May 18, 2018

vsemozhetbyt left a comment

vsemozhetbyt May 18, 2018 •

edited

Loading

BridgeAR May 18, 2018

BridgeAR May 19, 2018

vsemozhetbyt May 18, 2018

BridgeAR May 18, 2018

vsemozhetbyt May 18, 2018

vsemozhetbyt May 18, 2018

BridgeAR commented May 19, 2018 •

edited

Loading

vsemozhetbyt commented May 19, 2018

BridgeAR commented May 19, 2018 •

edited

Loading

Trott commented May 20, 2018

BridgeAR commented May 21, 2018

		@@ -1162,8 +1110,11 @@ util.inspect(process);

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

util: remove custom inspection function #20722

util: remove custom inspection function #20722

Conversation

BridgeAR commented May 14, 2018

Checklist

BridgeAR commented May 14, 2018 • edited Loading

mscdex May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented May 15, 2018

mcollina left a comment

Choose a reason for hiding this comment

BridgeAR commented May 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented May 18, 2018

BridgeAR commented May 18, 2018

vsemozhetbyt left a comment

Choose a reason for hiding this comment

vsemozhetbyt May 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented May 19, 2018 • edited Loading

vsemozhetbyt commented May 19, 2018

BridgeAR commented May 19, 2018 • edited Loading

Trott commented May 20, 2018

BridgeAR commented May 21, 2018

BridgeAR commented May 14, 2018 •

edited

Loading

mscdex May 14, 2018 •

edited

Loading

vsemozhetbyt May 18, 2018 •

edited

Loading

BridgeAR commented May 19, 2018 •

edited

Loading

BridgeAR commented May 19, 2018 •

edited

Loading