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.

Sign up for GitHub

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

Jump to bottom

module: rename anonymous functions #13849

Closed
wants to merge 2 commits into from
Closed

module: rename anonymous functions #13849

wants to merge 2 commits into from

Conversation

ranstyr
Copy link

@ranstyr ranstyr commented Jun 21, 2017 •
edited
Loading

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-bot nodejs-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
lib/module.js
@@ -635,7 +635,7 @@ Module._initPaths = function() {

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) {
Copy link
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).

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

@tniessen tniessen self-assigned this Jun 22, 2017
@benjamingr benjamingr mentioned this pull request Jun 22, 2017
Closed
@XadillaX
Copy link
Contributor

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

@Trott
Copy link
Member

Trott commented Jul 20, 2017

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

Closing due to long inactivity.

@BridgeAR BridgeAR closed this Aug 30, 2017
@BridgeAR
Copy link
Member

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

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

@mscdex mscdex mscdex left review comments

@mcollina mcollina mcollina left review comments

Assignees

@tniessen tniessen

Labels
module Issues 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