New max-lines-per-function rule

[dead-claudia]

Please describe what the rule should do: It should allow a configurable limit of the max number of lines per function, much like how max-statements limits the number of statements. Also, there should be an "ignoreComments" boolean option, on by default, that skips comments when counting method length.

What category of rule is this? (place an "X" next to just one item)

[x] Enforces code style
[ ] Warns about a potential error
[ ] Suggests an alternate way of doing something
[ ] Other (please specify:)

Provide 2-3 code examples that this rule will warn about:

// eslint max-lines-per-function: ["error", 1]
// Good
function foo() { return 1 }
const bar = () => 2

// Bad
function foo() {
    return 1
}
const bar = () =>
    2

// eslint max-lines-per-function: ["error", 5, {ignoreComments: false}]
// Good
function foo() {
    // This is a multi-line
    // comment.
    doSomething();
}

// Bad
function foo() {
    // This is a multi-line
    // comment.
    doSomething();
    doSomethingElse();
}

function foo() {
    // This is a multi-line
    // comment that takes
    // three lines.
    doSomething();
}

// eslint max-lines-per-function: ["error", 3, {ignoreComments: true}]
// Good
function foo() {
    // This is a multi-line
    // comment.
    doSomething();
}

// Bad
function foo() {
    // This is a multi-line
    // comment.
    doSomething();
    doSomethingElse();
}

function foo() {
    doSomething();
    doSomethingElse();
}

Why should this rule be included in ESLint (instead of a plugin)?

It is a simple, basic rule similar to what already exists, and is a natural extension of a few of them. It's also very widely generic, and although it could replace max-statements in some uses, it checks a different metric. It also does not conflict with any other rules, and it's pretty much independent from everything short of the language itself.

Other linters and style guides in various languages already do similar:

• Rubocop has Metrics/MethodLength which does exactly what I propose here, including the proposed option.
• Google's JS style guide (by proxy of their C++ style guide), recommends 40 lines or fewer per function.
• The Linux Kernel's style guide recommends 48 lines or fewer.

This is the original bug I filed

It should basically allow a configurable limit of the max number of lines per function, much like how max-statements limits the number of statements. This has precedent: Google's JS style guide (by proxy of their C++ style guide), recommends 40 lines or fewer per function. Also, to draw another language's style guide, the Linux Kernel recommends 48 lines or fewer.

[eslint-deprecated]

Hi @isiahmeadows, thanks for the issue. It looks like there's not enough information for us to know how to help you.

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.

Requesting a rule change? Please see Proposing a Rule Change for instructions.

If it's something else, please just provide as much additional information as possible. Thanks!

[not-an-aardvark]

Closing because not enough information was provided to triage this issue.

[dead-claudia]

@not-an-aardvark What other information is needed?

[platinumazure]

Hi @isiahmeadows, it would really help us to have the information requested in this set of guidelines: Proposing a New Rule. Thanks!

[dead-claudia]

@platinumazure I updated the original bug to include all that info.

[platinumazure]

@isiahmeadows Thank you! I'll go ahead and reopen this issue.

[platinumazure]

@isiahmeadows Very sorry for the long delay on this. Just wanted to ask, why is max-statements not sufficient for this sort of thing? (I realize that the counting is different-- statements vs lines-- but I'm asking why it's insufficient, not why it's different.)

I think I can probably get behind this, but I want to make sure there's a compelling case for implementing a line-based rule here when we should already have one for statements.

[dead-claudia]

@platinumazure Some expressions (think: long method chains, JSX/hyperscript trees, functional pipelines) could take several lines, and max-statements would see only a few statements.

For a concrete example, in this method:

• max-statements would see exactly one statement, even though it's 25 lines long.
• max-depth would see no nested blocks, so it wouldn't be a limiting factor here, either.
• max-nested-callbacks would see a depth of at most 3, which is much lower than what most care about (default is 10, but I keep it at 4 for my personal projects).
• complexity is 1, so it has little-to-no use here.

Another concrete example, this method:

• max-statements would only see two statements, even though it's 36 lines long (a large method by any standard).
• max-depth would see a depth of at most 1 - there's no blocks, just calls and function expressions.
• max-nested-callbacks would see a depth of at most 3.
• The only rule that would have much of an effect limiting it would be complexity, which would rate the outermost at 7, the .map callback at 3, and the rest at 1 (default is 10).

So there's uses for it, just in the more pathological cases. (Of course, most code doesn't have this issue.)

[platinumazure]

Alright, thanks, I'm sold. TBH I'm not going to push hard for this with the team right now since we're doing major release candidates, but I'll champion this after we finish that off. (That's a promise but real life does get in the way- feel free to ping me if you think I might need a reminder. Thanks!)

[platinumazure]

As promised, I'll champion this.

[platinumazure]

@eslint/eslint-team Could we get two more 👍s on this? This seems like a worthwhile addition since it complements other "max" rules which enforce readability.

[mysticatea]

How does it behave in the following case?

function foo(
    aaa = 1,
    bbb = 2,
    ccc = 3,
) {
    return aaa + bbb + ccc
}

I guess that the rule handles function head and body separately.

[peteward44]

In your example it will report 7 lines (it includes the head). The rule listens to the FunctionDeclaration node in the AS tree.

[platinumazure]

Seems to me like we could start with one approach or the other, and then enhance the rule to support the other use case later if needed. 😄

[mysticatea]

Though I felt natural if it counts only function bodies. But I'm OK if the lines of whole functions is more popular.

[alberto]

How would it treat IIFEs and nested functions?

[peteward44]

As it works at the moment, nested functions are included in their parent's count. Nested functions are also counted individually (so if a nested function exceeds the limit, both it and it's parent will fail).

For IIFEs the rule will just ignore the surrounding parentheses; so the following will report 4 lines:

(
function
()
{
}
)
()

Literally all the rule does is look for "function" (or starting parentheses for fat arrow) and the last curly bracket then count the line endings between them.

I will extend the unit tests to include the examples you guys have raised though.

[mysticatea]

Ahh, and how about class methods? Actually, a part of class methods is FunctionExpression, but some modifiers and method names are not a part of FunctionExpression.

I guess currently...:

class A {
    static
    foo
    // This FunctionExpression starts from below `(`, so this is 3.
    (a) {
        return a
    }
}

// Getters/setters are similar to it.
var obj = {
    get
    foo
    // This FunctionExpression starts from below `(`, so this is 3.
    () {
        return 1
    }
}

// The computed property cases can be longer.
class A {
    static
    [
        foo +
            bar
    ]
    // This FunctionExpression starts from below `(`, so this is 3.
    (a) {
        return a
    }
}

I think more consistent if this rule behaves either:

1. ignores methods or
2. counts lines from the top of Property/MethodDefinition nodes.

[peteward44]

Yep, i've just tested those examples and you are right. Ignoring methods seems to defeat the point of the rule so i'm going to try your other suggestion.

[ljharb]

Having boolean options that don't default to false is super weird; although I'd prefer ignoreIIFEs to default to false, if you want the default to be "ignore IIFEs", then can we pick an option name that makes sense when defaulting to false?

[platinumazure]

I think @ljharb is suggesting that it might be better to have an "opt in" option name such as enforceForIIFEs. I kind of agree.

[ljharb]

Yes - basically that for any boolean option, absence and false should be the same, for maximum clarity.

[peteward44]

OK i've updated the PR with all the feedback so far. Thanks all. As far as whether IIFEs should be included by default or not i'll go with whatever you guys want; but i agree with @not-an-aardvark that it would cause usability issues, and i'm of the opinion that default values should generally be the most user-friendly possible 🤷‍♂️

[ljharb]

My opinion is that defaults should be as restrictive as possible as well, so users are loosening them with intention.

[platinumazure]

Now that we have a champion and three 👍 from the team, this is accepted.