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

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

Closed
wants to merge 2 commits into
base: master
from

Conversation

@targos
Member

targos commented Feb 7, 2017

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

tools: enable ES2017 syntax support in ESLint
This allows us to use async functions.

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

Show outdated Hide outdated lib/util.js Outdated
Show outdated Hide outdated test/parallel/test-util-inspect.js Outdated
@bnoordhuis

LGTM with a style nit.

Show outdated Hide outdated lib/util.js Outdated
Show outdated Hide outdated test/parallel/test-util-inspect.js Outdated
@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Feb 7, 2017

Member

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

Member

targos commented Feb 7, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Feb 7, 2017

Member

Seems fine to me.

Member

bnoordhuis commented Feb 7, 2017

Seems fine to me.

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Feb 7, 2017

Member

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

Member

targos commented Feb 7, 2017

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

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

Show outdated Hide outdated lib/util.js Outdated
Show outdated Hide outdated lib/util.js Outdated
@evanlucas

LGTM with @addaleax's suggestion

@evanlucas

This comment has been minimized.

Show comment
Hide comment
@evanlucas

evanlucas Feb 7, 2017

Member

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

Member

evanlucas commented Feb 7, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@targos

targos Feb 7, 2017

Member

@evanlucas I can do that

Member

targos commented Feb 7, 2017

@evanlucas I can do that

@addaleax

Thanks for bearing with me. LGTM! :)

@targos

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

LGTM with style nits.

@@ -400,11 +400,14 @@ function formatValue(ctx, value, recurseTimes) {
});
}
var constructor = getConstructorOf(value);

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 7, 2017

Member

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

@bnoordhuis

bnoordhuis Feb 7, 2017

Member

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

This comment has been minimized.

@targos

targos Feb 7, 2017

Member

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

@targos

targos Feb 7, 2017

Member

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

Show outdated Hide outdated lib/util.js Outdated
Show outdated Hide outdated lib/util.js Outdated
util: improve inspect for (Async|Generator)Function
Use the constructor name in the output, if present.

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

util: improve inspect for AsyncFunction
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

This comment has been minimized.

Show comment
Hide comment
@targos

targos Feb 7, 2017

Member

PR to v7.x-staging: #11211

Member

targos commented Feb 7, 2017

PR to v7.x-staging: #11211

@cjihrig

cjihrig approved these changes Feb 7, 2017

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

This comment has been minimized.

@joyeecheung

joyeecheung Feb 7, 2017

Member

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)

@joyeecheung

joyeecheung Feb 7, 2017

Member

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)

@jasnell

jasnell approved these changes Feb 7, 2017

@refack refack referenced this pull request Feb 7, 2017

Closed

Got node to build on VS2017 #33

@TimothyGu

Thanks a lot. LGTM.

@addaleax addaleax referenced this pull request Feb 8, 2017

Closed

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

3 of 3 tasks complete

jasnell added a commit that referenced this pull request Feb 11, 2017

tools: enable ES2017 syntax support in ESLint
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>

jasnell added a commit that referenced this pull request Feb 11, 2017

util: improve inspect for (Async|Generator)Function
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

This comment has been minimized.

Show comment
Hide comment
@jasnell
Member

jasnell commented Feb 11, 2017

Landed in 615789b...f65aa08

@jasnell jasnell closed this Feb 11, 2017

jasnell added a commit that referenced this pull request Feb 13, 2017

util: improve inspect for AsyncFunction
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>

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

util: improve inspect for AsyncFunction
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>

KryDos added a commit to KryDos/node that referenced this pull request Feb 25, 2017

tools: enable ES2017 syntax support in ESLint
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>

KryDos added a commit to KryDos/node that referenced this pull request Feb 25, 2017

util: improve inspect for (Async|Generator)Function
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>

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

@targos targos deleted the targos:util-async-func branch Jun 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment