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

`max-statements` should have option to ignore top-level function scope #4309

Closed
isiahmeadows opened this Issue Nov 1, 2015 · 16 comments

Comments

Projects
None yet
7 participants
@isiahmeadows
Copy link

isiahmeadows commented Nov 1, 2015

The UMD/IIFE wrapper is a common pattern, but it's inconvenient to have to disable max-statements for the first line of the wrapper each time as a workaround for this problem. Example:

/* eslint max-statements: [2, 20] */
(function (global) { // eslint-disable-line max-statements
  // code with most likely too many statements...
})(typeof window !== 'undefined' ? window : this);

There should be an option to ignore the top-level function scope, e.g. the scope that second comment is in, so the IIFE and UMD patterns can be better accommodated. Maybe like "ignoreTopLevelFunctions": true or something to that effect?

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Nov 1, 2015

Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@eslintbot eslintbot added the triage label Nov 1, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 2, 2015

Why don't you increase your maximum by 1?

@isiahmeadows

This comment has been minimized.

Copy link
Author

isiahmeadows commented Nov 2, 2015

Are you talking about increasing the linter setting, or am I missing an
option that isn't documented?

On Mon, Nov 2, 2015, 11:13 Nicholas C. Zakas notifications@github.com
wrote:

Why don't you increase your maximum by 1?


Reply to this email directly or view it on GitHub
#4309 (comment).

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 3, 2015

You have /* eslint max-statements: [2, 20] */. I'm asking why you don't change it to /* eslint max-statements: [2, 21] */?

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Nov 3, 2015

@nzakas He's asking for the rule to not apply. Of course it would be sufficient to disable the rule or increase the limit, but I'm sure this would have to be done for each file that follows this pattern.

@isiahmeadows

This comment has been minimized.

Copy link
Author

isiahmeadows commented Nov 3, 2015

@michaelficarra Correct. My current workaround (recently used in the Mithril micro-framework) entails the above. It would also be useful for test cases, which below the first level should generally be focused.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 3, 2015

@michaelficarra I know what he's asking for, I'm trying to see if there's another option.

@impinball are you saying you set a different max inside of each file?

@isiahmeadows

This comment has been minimized.

Copy link
Author

isiahmeadows commented Nov 3, 2015

No. I'm asking for something I can throw into an eslintrc. The reason is
for the UMD pattern and similar (like tests).

On Tue, Nov 3, 2015, 11:31 Nicholas C. Zakas notifications@github.com
wrote:

@michaelficarra https://github.com/michaelficarra I know what he's
asking for, I'm trying to see if there's another option.

@impinball https://github.com/impinball are you saying you set a
different max inside of each file?


Reply to this email directly or view it on GitHub
#4309 (comment).

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 5, 2015

If that's the case, then why doesn't increasing the limit by one work?

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Nov 5, 2015

@nzakas I still don't think you're understanding the request. You're asking for @impinball to set a larger, homogenous maximum. The request is to have the rule apply differently in different contexts (top-level IIFE / everywhere else). With the proposed configuration, the inline override in the example wouldn't be there at all.

@IanVS IanVS added enhancement rule evaluating and removed triage labels Nov 10, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 11, 2015

@michaelficarra I understand what's being requested. He's setting a maximum statement count that applies to all files and wants to be able to do that despite the UMD wrapper. So right now he's manually disabling the rule for just that line. Effectively, that increases the maximum statement count by one, and I'm asking why not just do that in the configuration file using the already-existing option.

We're basically talking about an off-by-one error here, so I'm having a hard time understanding why increasing the maximum limit by one doesn't solve this problem.

@isiahmeadows

This comment has been minimized.

Copy link
Author

isiahmeadows commented Nov 12, 2015

@nzakas No, it's not an off-by-one. And comments don't count towards the statement count. They are attached separately to the AST by nearly every parser out there.

You're still not quite there: the difference between the current option and what I'm requesting is the difference between these:

  • Current option:

    /* eslint max-statements: [2, 2] */
    (function (global) {
      global.foo = function () {
        x.foo()
        x.bar()
        x.baz() // Error!
      }
      global.bar = function () {}
      global.baz = function () {} // Error!
    })(this)
  • What I would like:

    /* eslint max-statements: [2, 2, "ignore-first"] */
    (function (global) {
      global.foo = function (x) {
        x.foo()
        x.bar()
        x.baz() // Error!
      }
      global.bar = function () {}
      global.baz = function () {} // Okay.
    })(this);

You understand what I mean better?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 12, 2015

@impinball yes, I understand what you're requesting, I'm just having a hard time understanding why increasing your max statements by 1 doesn't solve your problem. Here's what I think I know:

  1. You want a statement not to count towards the maximum.
  2. You want this setting to be in a config file, not in each source file.

Is any of that incorrect?

What you're asking for, effectively, is to increase the maximum statement count by one for all files with a top-level function. What I'm suggesting is just to increase your maximum by one manually to get the same effect, since that is your effective maximum statement count.

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Nov 12, 2015

@nzakas I don't think that's going to work. It won't scale. Take a look at the example below, supposing he wants 2 statements per block except on the outermost function expression:

/* eslint max-statements: [2, 2, "ignore-first"] */
(function (global) {
  global.foo = function (x) {
    x.foo()
    x.bar()
    x.baz() // Error! (This is correct)
    x.bat() // Error! (This is also correct)
  }
  global.bar = function () {}
  global.baz = function () {} // Error! (This is not what he wants.)
  global.someOtherFunction = function () {} // Error! (Also not what he wants.)
  global.yetAnotherFunction = function () {} // Error! (Still not what he wants.)
})(this);

In this example, he is basically exporting a bunch of functions to global, and he wants no limit on the number of statements in that outermost IIFE since each statement is just a simple property assignment. But he DOES want a limit on the inner functions, because each function should be simple and have a maximum number of statements.

Increasing the max by 1 won't be sufficient, and incresaing the max to whatever the outermost IIFE needs will have the undesired effect of allowing the number of statements in the inner functions to grow as well.

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Nov 12, 2015

@nzakas Maybe if we think of statement lists like arrays.

[ 0, 1, [ 2, 3, [ 4, 5, [ 6, 7, 8 ] ] ] ]

Notice how none of the above arrays are, individually, longer than 3 elements. The exception that is being requested here is not that any of the arrays are allowed to be longer than 3 elements. It is that the outermost array, containing 0, 1, and another array, be allowed to have a different or even arbitrary maximum length. This is useful because of patterns like UMD that exist today.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 13, 2015

@platinumazure @michaelficarra ah! Thank you, guys. I now realize my confusion is that I forgot max-statements operates on individual functions. In that context, this request makes sense.

@nzakas nzakas added accepted and removed evaluating labels Nov 13, 2015

alberto added a commit that referenced this issue Dec 17, 2015

alberto added a commit that referenced this issue Dec 18, 2015

@alberto alberto self-assigned this Dec 19, 2015

alberto added a commit that referenced this issue Dec 20, 2015

@alberto alberto closed this in 6e04c00 Dec 21, 2015

nzakas added a commit that referenced this issue Dec 21, 2015

Merge pull request #4733 from eslint/issue4309
Update: option to ignore top-level max statements (fixes #4309)

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

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.