Skip to content

lib: remove dollar symbol for private function '$getMaxListeners'#25590

Closed
ghost wants to merge 1 commit intomasterfrom
unknown repository
Closed

lib: remove dollar symbol for private function '$getMaxListeners'#25590
ghost wants to merge 1 commit intomasterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Jan 20, 2019 •
edited by ghost
Loading

Just remove '$' because this isn't a programming language like Python.

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

@nodejs-github-bot nodejs-github-bot added the events Issues and PRs related to the events subsystem / EventEmitter. label Jan 20, 2019
@ghost ghost changed the title lib: Remove dollar symbol for private function lib: remove dollar symbol for private function '$getMaxListeners' Jan 20, 2019
@devsnek
Copy link
Copy Markdown
Member

devsnek commented Jan 20, 2019

this doesn't really need an underscore either

lib: remove dollar symbol for private function
fe9e1ce 
Just remove '$' because this isn't a programming language like Python.
@addaleax
Copy link
Copy Markdown
Member

addaleax commented Jan 27, 2019

@Maledong I think that’s because of the second getMaxListeners in EventEmitter.prototype.getMaxListeners = function getMaxListeners() { .... I think we can just remove that too.

@devsnek
Copy link
Copy Markdown
Member

devsnek commented Jan 27, 2019

@addaleax iirc the name was added as part of a code+learn to help debugging through core, might be good to keep it.

@addaleax
Copy link
Copy Markdown
Member

addaleax commented Jan 28, 2019

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

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 28, 2019
@danbev
Copy link
Copy Markdown
Contributor

danbev commented Jan 29, 2019

Re-run of failing node-test-commit-arm-fanned

@joyeecheung
Copy link
Copy Markdown
Member

joyeecheung commented Jan 29, 2019 •
edited
Loading

iirc the name was added as part of a code+learn to help debugging through core, might be good to keep it.

I believe v8 is already capable of inferring function names like this (assigned through a = so it's pretty trivial for the stack-based inferrer)? (And with even more additional context when collecting error stack traces, e.g. the original name and the new name if they are different)

@addaleax
Copy link
Copy Markdown
Member

addaleax commented Feb 8, 2019

Landed in 3a4521a

@addaleax addaleax closed this Feb 8, 2019
addaleax pushed a commit that referenced this pull request Feb 8, 2019
@addaleax
lib: remove dollar symbol for private function
3a4521a 
Just remove '$' because this isn't a programming language like Python.

PR-URL: #25590
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 8, 2019
@addaleax
lib: remove dollar symbol for private function
1b8d2ca 
Just remove '$' because this isn't a programming language like Python.

PR-URL: #25590
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@ghost ghost deleted the RemoveDollarSymbolForPrivateFunction branch February 9, 2019 01:15
@ghost
Copy link
Copy Markdown
Author

ghost commented Feb 9, 2019

Thanks!

@targos targos mentioned this pull request Feb 14, 2019
Merged
This was referenced Feb 15, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. events Issues and PRs related to the events subsystem / EventEmitter.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@devsnek @addaleax @danbev @joyeecheung @jasnell @thefourtheye @antsmartian @cjihrig @nodejs-github-bot