More realistic custom inspect example #8875

Havvy · 2016-10-01T01:21:56Z

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

Description of change

Changes the custom inspect example to a more complex object that
actually uses the parameters of the custom inspect function. I
specifically chose a wrapper of a value called a "Box" that inspects
to "Box < inner_inspect_value >".

I also want there to be documentation explaining what the code is
actually doing. E.g., the padding replacement part is to make the
inspected value line up properly when multi-line inputs are given.

I also went with having a space between the Box's brackets and the
inner value because it matches how objects work, and that should
definitely be listed as a convention somewhere in here.

Also, the convention to shorten only when depth is less than 0,
not e.g. at 0.

But I don't know how to write the documentation for that, so I'm
leaving that to somebody who reads this message.

Changes the custom inspect example to a more complex object that actually uses the parameters of the custom inspect function. I specifically chose a wrapper of a value called a "Box" that inspects to "Box < inner_inspect_value >". I also want there to be documentation explaining what the code is actually doing. E.g., the padding replacement part is to make the inspected value line up properly when multi-line inputs are given. I also went with having a space between the Box's brackets and the inner value because it matches how objects work, and that should definitely be listed as a convention somewhere in here. Also, the convention to shorten only when depth is less than 0, not e.g. at 0. But I don't know how to write the documentation for that, so I'm leaving that to somebody who reads this message. Partially fixes: nodejs#8442

Havvy · 2016-10-01T01:34:01Z

This is a partial fix for #8442.

Slayer95 · 2016-10-01T08:12:34Z

doc/api/util.md

+    });
+
+    // Five space padding because that's the size of "Box< ".
+    const padding = '     ';


This can be better written as ' '.repeat(5)

Done in the next commit. Thanks for pointing out that exists.

evanlucas · 2016-10-03T11:08:38Z

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

LGTM. Thanks for the contribution!

fhinkel · 2016-10-03T11:25:34Z

Thanks @Havvy. Do you want to take a stab at documenting the example?

addaleax

This looks pretty good, thank you!

addaleax · 2016-10-03T11:54:57Z

doc/api/util.md

-const obj = { name: 'nate' };
-obj[util.inspect.custom] = function(depth) {
-  return `{${this.name}}`;
-};


I’m not sure, so this is just a suggestion, but maybe leaving the “simpler” example first would be helpful? The Box example is good but maybe a bit overwhelming on the first look?

If we keep it, then I'd argue that we'd want to also add text saying it's a minimal example.

@Havvy Agreed. Maybe subheadings that say something like:

A simple example

A more realistic example

Havvy · 2016-10-04T01:06:54Z

@fhinkel I don't know how to properly document the example in a way that would help new people learn.

jasnell · 2016-10-06T16:58:47Z

doc/api/util.md


-util.inspect(obj);
-  // "{nate}"
+  inspect(depth, options) {


@addaleax ... what do you think.. should this be modified to use the new symbol you introduced as an alternative to using inspect directly?

@jasnell We could do that, but the motivation for introducing the symbol-based alternative were objects that have a regular inspect property which would conflict with what Node expects, and that doesn’t really apply to this use case. And as long as v4.x is supported, just using inspect is probably what I would do here, too. (But yeah, it’s hard to explain that situation concisely in an example).

jasnell

LGTM

jasnell · 2016-10-06T22:08:48Z

doc/api/util.md


-util.inspect(obj);
-  // "{nate}"
+  inspect(depth, options) {


@Havvy .. as a suggestion per @fhinkel's request, a code comment in here that describes the basic flow would be helpful.

Will do when I have time and energy.

jasnell · 2016-10-06T22:09:19Z

doc/api/util.md

-  // "{nate}"
+  inspect(depth, options) {
+    if (depth < 0) {
+      return options.stylize('[Box]', 'special');


@Havvy ... likewise here, a comment that describes briefly what options.stylize does.

jasnell · 2016-10-06T22:09:43Z

doc/api/util.md

+
+    // Five space padding because that's the size of "Box< ".
+    const padding = ' '.repeat(5);
+    const inner = util.inspect(this.value, newOptions).replace(/\n/g, '\n' + padding);


And a comment here about what this call to util.inspect is for.

Will do when I have time and energy.

jasnell · 2016-10-06T22:11:03Z

doc/api/util.md

+    // Five space padding because that's the size of "Box< ".
+    const padding = ' '.repeat(5);
+    const inner = util.inspect(this.value, newOptions).replace(/\n/g, '\n' + padding);
+    return options.stylize('Box', 'special') + '< ' + inner + ' >';


I know I already signed off on this but, just as a very minor nit, this could be slightly simplified to:

return `${options.stylize('Box', 'special')}<${inner}>`;

I am not sure I’d find that more readable… long expressions inside ${} are a bit weird imho.

So, maybe options.stylize('Box', 'special') + < ${inner} >;?

Do we really want to mix quasiquoting and string concatenation together?

Seems like any option we go with (other than just string concatenation IMO) looks weird.

addaleax · 2016-10-25T16:35:20Z

Given that this PR has been open for a while and there are sign-offs on it in its current state, I’m inclined to say “let’s land this and take care of minor adjustments afterwards if still necessary/desired”.

Havvy · 2016-10-25T19:40:09Z

My computer broke, and the one I'm on is too poor to try to do anything, so I haven't been able to anything recently. Sorry. If you want to push this now, it's still better than what we currently have.

addaleax · 2016-10-25T19:43:08Z

@Havvy Sorry to hear that! ☹️

If there are specific changes, you can enable the Allow edits from maintainers checkbox here and ask us to carry them out, but yes, I think this is a big improvement as it is.

If nobody objects, I’d like to land this by Thursday.

addaleax · 2016-10-26T20:35:19Z

Landed in 97dfced

Changes the custom inspect example to a more complex object that actually uses the parameters of the custom inspect function. I specifically chose a wrapper of a value called a "Box" that inspects to "Box < inner_inspect_value >". I also want there to be documentation explaining what the code is actually doing. E.g., the padding replacement part is to make the inspected value line up properly when multi-line inputs are given. I also went with having a space between the Box's brackets and the inner value because it matches how objects work, and that should definitely be listed as a convention somewhere in here. Also, the convention to shorten only when depth is less than 0, not e.g. at 0. But I don't know how to write the documentation for that, so I'm leaving that to somebody who reads this message. Ref: #8442 PR-URL: #8875 Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

nodejs-github-bot added doc Issues and PRs related to the documentations. util Issues and PRs related to the built-in util module. labels Oct 1, 2016

Havvy force-pushed the inspect-example branch from 989154d to bc57bc8 Compare October 1, 2016 01:31

Slayer95 reviewed Oct 1, 2016

View reviewed changes

use " ".repeat in custom util.inspect example

ad9ef92

addaleax approved these changes Oct 3, 2016

View reviewed changes

jasnell reviewed Oct 6, 2016

View reviewed changes

jasnell approved these changes Oct 6, 2016

View reviewed changes

jasnell reviewed Oct 6, 2016

View reviewed changes

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

addaleax mentioned this pull request Oct 25, 2016

intl: Add more versions from ICU #9266

Closed

4 tasks

addaleax closed this Oct 26, 2016

evanlucas mentioned this pull request Nov 3, 2016

v7.1.0 proposal - 2016-11-08 #9438

Merged

MylesBorins added the dont-land-on-v4.x label Nov 17, 2016

MylesBorins added the land-on-v6.x label Nov 17, 2016

MylesBorins mentioned this pull request Nov 22, 2016

v6.9.2 proposal #9735

Merged

addaleax mentioned this pull request Apr 30, 2017

How do I implement MyObject.inspect(depth, inspectOpts)? #8442

Closed

aliscco mentioned this pull request Nov 29, 2023

[Snyk] Security upgrade eslint from 2.13.1 to 4.5.0 aliscco/alisco-node#705

Open

leonardoadame mentioned this pull request Nov 29, 2023

[Snyk] Fix for 4 vulnerabilities leonardoadame/node#246

Open

This was referenced Nov 30, 2023

[Snyk] Security upgrade eslint from 2.13.1 to 4.5.0 aliscco/alisco-node#826

Open

[Snyk] Fix for 3 vulnerabilities aliscco/alisco-node#846

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More realistic custom inspect example #8875

More realistic custom inspect example #8875

Havvy commented Oct 1, 2016

Havvy commented Oct 1, 2016

Slayer95 Oct 1, 2016 •

edited

Loading

Havvy Oct 2, 2016

evanlucas commented Oct 3, 2016

fhinkel commented Oct 3, 2016

addaleax left a comment

addaleax Oct 3, 2016

Havvy Oct 4, 2016

gibfahn Oct 4, 2016

Havvy commented Oct 4, 2016

jasnell Oct 6, 2016

addaleax Oct 6, 2016

jasnell left a comment

jasnell Oct 6, 2016

Havvy Oct 8, 2016

jasnell Oct 6, 2016

jasnell Oct 6, 2016

Havvy Oct 8, 2016

jasnell Oct 6, 2016 •

edited

Loading

addaleax Oct 6, 2016

Havvy Oct 8, 2016

addaleax commented Oct 25, 2016

Havvy commented Oct 25, 2016

addaleax commented Oct 25, 2016

addaleax commented Oct 26, 2016

More realistic custom inspect example #8875

More realistic custom inspect example #8875

Conversation

Havvy commented Oct 1, 2016

Checklist

Affected core subsystem(s)

Description of change

Havvy commented Oct 1, 2016

Slayer95 Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanlucas commented Oct 3, 2016

fhinkel commented Oct 3, 2016

addaleax left a 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

A simple example

A more realistic example

Havvy commented Oct 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell left a 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

jasnell Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Oct 25, 2016

Havvy commented Oct 25, 2016

addaleax commented Oct 25, 2016

addaleax commented Oct 26, 2016

Slayer95 Oct 1, 2016 •

edited

Loading

jasnell Oct 6, 2016 •

edited

Loading