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

module: rename anonymous functions #20021

Closed
wants to merge 4 commits into from
Closed

module: rename anonymous functions #20021

wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Apr 14, 2018

This is my first contribution 👋
#8913

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the domain Issues and PRs related to the domain subsystem. label Apr 14, 2018
@lpinca
Copy link
Member

lpinca commented Apr 14, 2018

Please see discussion in original issue #8913 (comment). I think consensus was to not name function on prototypes as the inferred name is actually better.

cc: @mcollina @Trott

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Unfortunately, none of these functions needs to be renamed. Everything is assigned to a prototype is generally ok.

@apapirovski
Copy link
Member

@mcollina I don't think that's correct. The first function should still be renamed?

@mcollina
Copy link
Member

Oh I just saw the others. yes, the first function should be renamed! Good spot!

@ghost
Copy link
Author

ghost commented Apr 14, 2018

Alright i'll keep that first function 👍

@trivikr
Copy link
Member

trivikr commented Apr 14, 2018

@mcollina Should the names assigned to prototype functions (like from commit 25be2a3) in other files be removed?

@ghost
Copy link
Author

ghost commented Apr 14, 2018

I see this one too, https://github.com/nodejs/node/pull/20021/files#diff-0b93f5d72b0d34f5b972b661935344c3L196 in domain.js. That can be can be removed right?

@lpinca
Copy link
Member

lpinca commented Apr 14, 2018

@dooven yes I think so.

@apapirovski
Copy link
Member

@dooven There's one more of those lower down in the file EventEmitter.prototype.emit = function emit(...args) {

@ghost
Copy link
Author

ghost commented Apr 14, 2018

Thanks. Will update that

@trivikr
Copy link
Member

trivikr commented Apr 14, 2018

@ghost
Copy link
Author

ghost commented Apr 14, 2018

@Trott
Copy link
Member

Trott commented Apr 14, 2018

Re-running Linux CI, although it looks like it's been a while since there's been a green one so something might be up: https://ci.nodejs.org/job/node-test-commit-linux/18005/

@Trott
Copy link
Member

Trott commented Apr 14, 2018

OK, I think refack fixed the failing-to-build Linux host. Let's try again.

Linux CI: https://ci.nodejs.org/job/node-test-commit-linux/18009/

@Trott
Copy link
Member

Trott commented Apr 15, 2018

Last Linux re-run was green. This is good to land if there are no objections from any Collaborators after 72 hours has passed.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 15, 2018
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

trivikr pushed a commit that referenced this pull request Apr 18, 2018
PR-URL: #20021
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@trivikr
Copy link
Member

trivikr commented Apr 18, 2018

Landed in ca41a30

Congratulations @dooven on your first commit to Node.js core! 🎉🎉🎉

@trivikr trivikr closed this Apr 18, 2018
jasnell pushed a commit that referenced this pull request Apr 19, 2018
PR-URL: #20021
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. domain Issues and PRs related to the domain subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants