util: improve inspect for (Async|Generator)Function #11210

targos · 2017-02-07T09:50:53Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

util

This allows us to use async functions.

addaleax · 2017-02-07T09:53:15Z

lib/util.js

(async function() {}).constructor.name === 'AsyncFunction', so I’d probably want to go with that? Or just generally actually use the constructor name, if it’s available?

Maybe. I didn't think about that. We don't do it for GeneratorFunction though. Should I extend the PR to support it too?

I mean, if I change the code to use constructor.name, this will be semver-major. I'm OK with that.

Style nit: indent by four spaces.

this will be semver-major

You mean, because it changes the output for GeneratorFunction? Yeah, that’s probably for the best. And yes, I think extending it to GeneratorFunctions would be a good idea, too.

TimothyGu · 2017-02-07T09:55:22Z

test/parallel/test-util-inspect.js

The original uses the capitalized constructor name (Function). How about using the same format, AsyncFunction?

Seems a bit redundant: async AsyncFunction

bnoordhuis

LGTM with a style nit.

bnoordhuis · 2017-02-07T10:01:23Z

lib/util.js

Style nit: indent by four spaces.

bnoordhuis · 2017-02-07T10:04:46Z

test/parallel/test-util-inspect.js

Seems a bit redundant: async AsyncFunction

targos · 2017-02-07T10:06:44Z

@bnoordhuis I'm changing the PR to get this output:

$ ./node > function*a(){} undefined > a [GeneratorFunction: a] > async function b(){} undefined > b [AsyncFunction: b]

What do you think?

bnoordhuis · 2017-02-07T10:12:06Z

Seems fine to me.

targos · 2017-02-07T10:18:15Z

OK, updated. Marking semver-major because the output changes for generator functions.

addaleax · 2017-02-07T10:21:29Z

lib/util.js

There’s a getConstructorOf utility inside this file for dealing with some edge cases :)

Aha! Thanks, updated.

addaleax · 2017-02-07T10:43:36Z

lib/util.js

Sorry to comment again 😄 I’d just move the constructor variable up before the keys.length block and do the same constructor && constructor.name … check that is used elsewhere in this file. I doubt there are many people who set their functions’ prototypes to null, but I’d prefer handling these consistently…

No problem :) Updated again.

evanlucas

LGTM with @addaleax's suggestion

evanlucas · 2017-02-07T11:27:26Z

@targos with this being a semver-major change, do you have plans to open a PR to do this for async functions in v7 as well (without the generator function change)?

targos · 2017-02-07T11:43:12Z

@evanlucas I can do that

addaleax

Thanks for bearing with me. LGTM! :)

targos · 2017-02-07T12:30:26Z

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

bnoordhuis

LGTM with style nits.

bnoordhuis · 2017-02-07T12:30:54Z

lib/util.js

Maybe const it while you're here. (EDIT: nevermind, I see it's being reassigned in some places.)

I can't make it const because the value may be changed later.

bnoordhuis · 2017-02-07T12:32:44Z

lib/util.js

I have an irrational dislike for the name cName. constructorName or ctorName if the former is too long?

(Applies to line 541 as well.)

Changed to ctorName

bnoordhuis · 2017-02-07T12:33:09Z

lib/util.js

Four spaces, please. :-)

Use the constructor name in the output, if present.

Use the constructor name in the output, if present. This is a backport of nodejs#11210 without the semver-major change to GeneratorFunction output.

targos · 2017-02-07T12:49:21Z

PR to v7.x-staging: #11211

joyeecheung · 2017-02-07T15:07:21Z

lib/util.js

- 'special');
+ const ctorName = constructor ? constructor.name : 'Function';
+ return ctx.stylize(
+ `[${ctorName}${value.name ? `: ${value.name}` : ''}]`, 'special');


Nested templates can be a little bit hard to read, maybe change it to use if-else? (though this LTGM too with this unchanged, now that there is a backport PR already)

TimothyGu

Thanks a lot. LGTM.

This allows us to use async functions. PR-URL: #11210 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

Use the constructor name in the output, if present. PR-URL: #11210 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

jasnell · 2017-02-11T21:34:57Z

Landed in 615789b...f65aa08

Use the constructor name in the output, if present. This is a backport of #11210 without the semver-major change to GeneratorFunction output. PR-URL: #11211 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

Use the constructor name in the output, if present. This is a backport of nodejs#11210 without the semver-major change to GeneratorFunction output. PR-URL: nodejs#11211 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

This allows us to use async functions. PR-URL: nodejs#11210 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

Use the constructor name in the output, if present. PR-URL: nodejs#11210 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

tools: enable ES2017 syntax support in ESLint

93db417

This allows us to use async functions.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. util Issues and PRs related to the built-in util module. labels Feb 7, 2017

targos changed the title ~~Util async func~~ util: emphasize inspected async functions Feb 7, 2017

targos added tools Issues and PRs related to the tools directory. dont-land-on-v4.x labels Feb 7, 2017

addaleax reviewed Feb 7, 2017

View reviewed changes

TimothyGu reviewed Feb 7, 2017

View reviewed changes

bnoordhuis approved these changes Feb 7, 2017

View reviewed changes

targos force-pushed the util-async-func branch from 1e28f15 to cdf7faf Compare February 7, 2017 10:17

targos added semver-major PRs that contain breaking changes and should be released in the next major version. and removed c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x labels Feb 7, 2017

targos changed the title ~~util: emphasize inspected async functions~~ util: improve inspect for (Async|Generator)Function Feb 7, 2017

addaleax reviewed Feb 7, 2017

View reviewed changes

targos force-pushed the util-async-func branch from cdf7faf to 88920b5 Compare February 7, 2017 10:34

addaleax reviewed Feb 7, 2017

View reviewed changes

evanlucas approved these changes Feb 7, 2017

View reviewed changes

targos force-pushed the util-async-func branch from 88920b5 to 686e91a Compare February 7, 2017 12:24

addaleax approved these changes Feb 7, 2017

View reviewed changes

bnoordhuis approved these changes Feb 7, 2017

View reviewed changes

util: improve inspect for (Async|Generator)Function

c2fac57

Use the constructor name in the output, if present.

targos force-pushed the util-async-func branch from 686e91a to c2fac57 Compare February 7, 2017 12:43

targos added a commit to targos/node that referenced this pull request Feb 7, 2017

util: improve inspect for AsyncFunction

bf85cf3

Use the constructor name in the output, if present. This is a backport of nodejs#11210 without the semver-major change to GeneratorFunction output.

cjihrig approved these changes Feb 7, 2017

View reviewed changes

joyeecheung approved these changes Feb 7, 2017

View reviewed changes

jasnell approved these changes Feb 7, 2017

View reviewed changes

refack mentioned this pull request Feb 7, 2017

Got node to build on VS2017 indutny/gyp.js#33

Closed

TimothyGu approved these changes Feb 7, 2017

View reviewed changes

addaleax mentioned this pull request Feb 8, 2017

v7.x: util: improve inspect for AsyncFunction #11211

Closed

3 tasks

jasnell closed this Feb 11, 2017

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

targos deleted the util-async-func branch June 1, 2017 07:37

Uh oh!

util: improve inspect for (Async|Generator)Function #11210

util: improve inspect for (Async|Generator)Function #11210

Conversation

targos commented Feb 7, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

targos Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Feb 7, 2017

bnoordhuis commented Feb 7, 2017

targos commented Feb 7, 2017

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

evanlucas left a comment

Choose a reason for hiding this comment

evanlucas commented Feb 7, 2017

targos commented Feb 7, 2017

addaleax left a comment

Choose a reason for hiding this comment

targos commented Feb 7, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

targos commented Feb 7, 2017

Choose a reason for hiding this comment

TimothyGu left a comment

Choose a reason for hiding this comment

jasnell commented Feb 11, 2017

Labels

9 participants

targos Feb 7, 2017 •

edited

Loading

bnoordhuis Feb 7, 2017 •

edited

Loading