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

getter-return marking method named 'get' as an issue #8919

Closed
jscharett opened this issue Jul 10, 2017 · 5 comments

Comments

Projects
None yet
6 participants
@jscharett
Copy link

commented Jul 10, 2017

  • ESLint Version: 4.2.0
  • Node Version: 4.4.3
  • npm Version: 2.15.11

What parser (default, Babel-ESLint, etc.) are you using?: default

The getter-return rule seems to catch any method that is called 'get' and does not have a return statement. The codebase I'm working on has a number of methods that are names get that take a callback and invoke the callback asynchrously. Its an older codebase developed before Promises were standard. This rule seems to catch those cases, which seems incorrect as they are not true 'getters'.

Example:
var x = { get: function(callback) { setTimeout(callback, 0); }};
x.get(function () {....});

@eslintbot eslintbot added the triage label Jul 10, 2017

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented Jul 10, 2017

I think the goal of this was to catch things like

Object.defineProperty(foo, 'bar', {
    get: function() {
        // no return statement
    }
});

But I agree that the current check seems overly strict, since it applies to any function named "get". If it's trying to detect errors in Object.defineProperty, it should actually make sure the object is part of an Object.defineProperty call.

@ilyavolodin

This comment has been minimized.

Copy link
Member

commented Jul 14, 2017

This should also check for Object.defineProperties as well as ES6 getter syntax.

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented Jul 14, 2017

It already checks getter syntax. (See here for how it decides whether to check a function.)

However, there is also another issue where it has a false negative for arrow functions in property descriptors:

Object.defineProperty(foo, 'bar', {get: () => {}})
@aladdin-add

This comment has been minimized.

Copy link
Member

commented Jul 24, 2017

I wrote this rule and this is intended, as the saying in the PR #8460

for code like this:

  var foo = { get: function () {bar();} };

it is not a getter function, but naming a method get that does not return a value, is so confusing, causing an error may be somewhat reasonable.

In most cases, I think it may be reasonable.

@platinumazure

This comment has been minimized.

Copy link
Member

commented Jul 24, 2017

I would be 👍 to only checking get functions that are part of Object.defineProperty or Object.defineProperties calls.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.