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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

ghost
Copy link

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

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

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
Contributor

danbev commented Jan 29, 2019

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

@joyeecheung
Copy link
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
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 <anto.aravinth.cse@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
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 <anto.aravinth.cse@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@ghost ghost deleted the RemoveDollarSymbolForPrivateFunction branch February 9, 2019 01:15
@ghost
Copy link
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