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

tools: enable getter-return lint rule #26615

Merged
merged 1 commit into from
Mar 26, 2019
Merged

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Mar 12, 2019

We use getters a lot throughout core. I think it makes sense to lint for them returning values.

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

@nodejs-github-bot nodejs-github-bot added dont-land-on-v6.x http2 Issues or PRs related to the http2 subsystem. net Issues and PRs related to the net subsystem. tools Issues and PRs related to the tools directory. labels Mar 12, 2019
@@ -492,7 +492,7 @@ Object.defineProperty(Socket.prototype, 'readyState', {


Object.defineProperty(Socket.prototype, 'bufferSize', {
get: function() {
get: function() { // eslint-disable-line getter-return
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, eslint-disable-line getter-return is unnecessary if this function returns undefined explicitly.

  get: function() {
    if (this._handle) {
      return this[kLastWriteQueueSize] + this.writableLength;
    }
    return undefined;
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shisama in this case I thought it would be simplest to just add the comment. If you feel strongly about it, I can add the return undefined; instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cjihrig No, I am not particular about it. This is just a proposal. Thanks for your reply :)

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 25, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 25, 2019

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

EDIT(cjihrig): CI was green.

PR-URL: nodejs#26615
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
@cjihrig cjihrig merged commit 5de804e into nodejs:master Mar 26, 2019
@cjihrig cjihrig deleted the getters branch March 26, 2019 00:06
BethGriggs pushed a commit that referenced this pull request Mar 26, 2019
PR-URL: #26615
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
PR-URL: nodejs#26615
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
targos pushed a commit that referenced this pull request Mar 27, 2019
PR-URL: #26615
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
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. http2 Issues or PRs related to the http2 subsystem. net Issues and PRs related to the net subsystem. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants