max-func-body-length fails to count body length correctly

[makotom]

Prerequisite

I assume that the following is a 3-line function:

(): void => {   // Line 1
                // Line 2
}               // Line 3

// I seriously think that it is an important definition. Without it, we (at least, I) can get confused. :P

Repro

1. git clone https://github.com/makotom/tslint-test-20180724.git
2. npm i
3. npm run lint

OR

1. Write a TypeScript code having the snippet I wrote in Prerequisite above.
2. Write a tslint.json and set rule "max-func-body-length": [true, 2]. This intends to raise an error if a function has more than 2 lines.
3. Run tslint

Expected Result

An error should be raised to a function having more than 2 lines.

Actual Result

No error.
Note that the test case I've provided (which is also referred in Repro) contains a 4-line function as well, to which an error is fired as expected.

Considerations

As I wrote in my testcase, this happens because it does not add 1 to endLine - startLine to calculate body length. I believe that it should add 1 - if not, it yields 0-line function if startLine === endLine satisfies (e.g. (x: number): number => x + 1).

Apparently, this can be fixed very easily. However, I found out that fixing it may break several tests which depends on error messages. I'm also afraid that the fix, which could be a single line, may break many other things. This is why I'm opning a new issue, rather than a PR, to call for discussions as the first step.

[JoshuaKGoldberg]

// I seriously think that it is an important definition. Without it, we (at least, I) can get confused. :P

Ah, but now I'm confused! Why is that an important definition for you? What's your use case for that particular interpretation?

Agreed that this should be a one-line function:

(x: number): number => x + 1

What about this?

(x: number): number => {
    return x + 1;
}

Perhaps the rule's calcBodyLength should consider the line difference between the start of the first statement to the end of the last statement?

[makotom]

Hi @JoshuaKGoldberg, thanks for your reply!

Why is that an important definition for you?
What's your use case for that particular interpretation?

Without this kind of definition, we cannot discuss which implementation is correct.

Fortunately, we have a good real example in https://github.com/Microsoft/tslint-microsoft-contrib/blob/master/src/tests/MaxFuncBodyLengthRuleTests.ts#L274:

describe('something', (): void => {
    // line 2
    // line 3
    // line 4
    // line 5
}); // line 6

Without that kind of definition I've given, we cannot say confidently whether this is four-line, five-line or six-line. (BTW, the current implementation considers it a five-line function.)

What about this?

(x: number): number => {
    return x + 1;
}

Hmmmmmm... Good point! I personally consider that this is a three-line function, but I would say that it is reasonable to name it a one-line function by considering "the line difference between the start of the first statement to the end of the last statement" as you say. Frankly speaking, I don't have a definite answer for this at this timing.

But I would say that should not be a two-line, whilst the current impelementation does say it is:

[JoshuaKGoldberg]

That makes sense, thanks!

Accepting PRs to have the rule consider both the () => { and } sections of the function as line counts.

To be clear, this should be considered to have 3 lines:

(x: number): number => {
    return x + 1;
}

...and this should have 1 line:

(x: number): number => x + 1

[JoshuaKGoldberg]

Fixed by #469! 🎉✨