Skip to content

module: rename anonymous functions - #13849

Closed
ranstyr wants to merge 2 commits into
nodejs:masterfrom
ranstyr:master
Closed

module: rename anonymous functions#13849
ranstyr wants to merge 2 commits into
nodejs:masterfrom
ranstyr:master

Conversation

@ranstyr

@ranstyrranstyr commented Jun 21, 2017•
edited
Loading

Copy link
Copy Markdown

This is my first contribution!
#GoodnessSquad
#8913

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • [X ] commit message follows commit guidelines
Affected core subsystem(s)

#GoodnessSquad

@nodejs-github-botnodejs-github-bot added the module Issues and PRs related to the module subsystem. label Jun 21, 2017
mscdex
mscdex reviewed Jun 21, 2017
View reviewed changes
Comment threadlib/module.js
var nodePath = process.env['NODE_PATH'];
if (nodePath) {
paths = nodePath.split(path.delimiter).filter(function(path) {
paths = nodePath.split(path.delimiter).filter(function pathsFilterCallback(path) {

@mscdexmscdexJun 21, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this is the only one that's necessary. The rest should show up as expected in stack traces, etc. because they're attached to an object (prototype or otherwise).

@mcollinamcollinaJun 21, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree, this is the only one that is needed. Can you please restore the other ones?

@benjamingr

benjamingr commented Jun 21, 2017

Copy link
Copy Markdown
Member

Nice work. Please feel free to also give us any feedback about the onboarding process today.

Note that JavaScript functions are automatically named when you assign them since ES2015.

LGTM on the change @mcollina approved, please amend the rest.

@tniessentniessen self-assigned this Jun 22, 2017
@benjamingrbenjamingr mentioned this pull request Jun 22, 2017
Closed
@XadillaX

XadillaX commented Jul 20, 2017

Copy link
Copy Markdown
Contributor

I think this PR could be closed due to #14297 (review).

@Trott

Trott commented Jul 20, 2017

Copy link
Copy Markdown
Member

I think this PR could be closed due to #14297 (review).

There's one function in module.js that needs to be named, so this can stay open in the hopes that the original submitter updates it or possibly an existing Collaborator can come along and update it.

@BridgeAR

BridgeAR commented Aug 30, 2017

Copy link
Copy Markdown
Member

Closing due to long inactivity.

@BridgeAR

BridgeAR commented Aug 30, 2017

Copy link
Copy Markdown
Member

@ranstyr please feel free to reopen if you want to follow up on this!

@ndanglesndangles mentioned this pull request May 18, 2018
Closed
2 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina left review comments
+1 more reviewer
@mscdexmscdexmscdex left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@tniessentniessen

Labels

moduleIssues and PRs related to the module subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@ranstyr@benjamingr@XadillaX@Trott@BridgeAR@mcollina@mscdex@tniessen@nodejs-github-bot