More realistic custom inspect example by Havvy · Pull Request #8875 · nodejs/node · GitHub

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

This is a partial fix for #8442.

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

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

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

LGTM. Thanks for the contribution!

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

This looks pretty good, thank you!

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

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

@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).

LGTM

@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.

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

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

Will do when I have time and energy.

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.

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”.

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.

@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.

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 <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

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

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

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

+                  // 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 + ' >';

More realistic custom inspect example by Havvy · Pull Request #8875 · nodejs/node · GitHub

Uh oh!

Uh oh!

More realistic custom inspect example - #8875

More realistic custom inspect example#8875Havvy wants to merge 2 commits intonodejs:masternodejs/node:masterfrom Havvy:inspect-exampleHavvy/node:inspect-exampleCopy head branch name to clipboard

Conversation

Havvy commented Oct 1, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Havvy commented Oct 1, 2016

Uh oh!

Slayer95 Oct 1, 2016 • 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!

evanlucas commented Oct 3, 2016

Uh oh!

fhinkel commented Oct 3, 2016

Uh oh!

addaleax left a comment

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

A simple example

A more realistic example

Uh oh!

Havvy commented Oct 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

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!

jasnell Oct 6, 2016 • 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!

addaleax commented Oct 25, 2016

Uh oh!

Havvy commented Oct 25, 2016

Uh oh!

addaleax commented Oct 25, 2016

Uh oh!

addaleax commented Oct 26, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

More realistic custom inspect example#8875
Havvy wants to merge 2 commits into
nodejs:masterfrom
Havvy:inspect-example

Slayer95 Oct 1, 2016 •
edited
Loading

jasnell Oct 6, 2016 •
edited
Loading