Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More realistic custom inspect example #8875

Closed
wants to merge 2 commits into from

Conversation

Havvy
Copy link
Contributor

@Havvy Havvy commented Oct 1, 2016

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
@nodejs-github-bot 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
Copy link
Contributor Author

Havvy commented Oct 1, 2016

This is a partial fix for #8442.

});

// Five space padding because that's the size of "Box< ".
const padding = ' ';
Copy link

@Slayer95 Slayer95 Oct 1, 2016

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@evanlucas
Copy link
Contributor

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

LGTM. Thanks for the contribution!

@fhinkel
Copy link
Member

fhinkel commented Oct 3, 2016

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

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

This looks pretty good, thank you!

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@Havvy Agreed. Maybe subheadings that say something like:

A simple example

A more realistic example

@Havvy
Copy link
Contributor Author

Havvy commented Oct 4, 2016

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


util.inspect(obj);
// "{nate}"
inspect(depth, options) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM


util.inspect(obj);
// "{nate}"
inspect(depth, options) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do when I have time and energy.

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

Choose a reason for hiding this comment

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

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


// 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);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do when I have time and energy.

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

@jasnell jasnell Oct 6, 2016

Choose a reason for hiding this comment

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

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}>`;

Copy link
Member

Choose a reason for hiding this comment

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

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} >;?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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

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

@addaleax addaleax mentioned this pull request Oct 25, 2016
4 tasks
@Havvy
Copy link
Contributor Author

Havvy commented Oct 25, 2016

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
Copy link
Member

@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
Copy link
Member

Landed in 97dfced

@addaleax addaleax closed this Oct 26, 2016
addaleax pushed a commit that referenced this pull request Oct 26, 2016
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>
evanlucas pushed a commit that referenced this pull request Nov 3, 2016
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>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2016
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>
MylesBorins pushed a commit that referenced this pull request Nov 19, 2016
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>
@MylesBorins MylesBorins mentioned this pull request Nov 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants