Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

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

Closed
makotom opened this issue Jul 24, 2018 · 4 comments
Closed

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

makotom opened this issue Jul 24, 2018 · 4 comments
Labels
Difficulty: Easy Someone with little to no experience in TSLint should be able to send a pull request for this issue. Status: Accepting PRs Type: Breaking Change Type: Bug
Milestone

Comments

@makotom
Copy link
Contributor

makotom commented Jul 24, 2018

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
Copy link

// 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?

@JoshuaKGoldberg JoshuaKGoldberg added Type: Question Status: In Discussion Please continue discussing the proposed change before sending a pull request. Status: Awaiting More Feedback and removed Type: Question labels Jul 24, 2018
@makotom
Copy link
Contributor Author

makotom commented Jul 24, 2018

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:
image

@JoshuaKGoldberg
Copy link

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 JoshuaKGoldberg added Type: Bug Status: Accepting PRs Difficulty: Easy Someone with little to no experience in TSLint should be able to send a pull request for this issue. Type: Breaking Change and removed Status: Awaiting More Feedback Status: In Discussion Please continue discussing the proposed change before sending a pull request. labels Jul 24, 2018
JoshuaKGoldberg pushed a commit that referenced this issue Jul 25, 2018
* Proposed fix for issue #468

* Revision of a test script responding to a fix of #468

* Avoid using tsutils v2.29.0, which breaks tests

* Revert "Avoid using tsutils v2.29.0, which breaks tests"

This reverts commit f1e963f.

See also: #469 (comment)
@JoshuaKGoldberg
Copy link

Fixed by #469! 🎉✨

@JoshuaKGoldberg JoshuaKGoldberg added this to the 5.2.0 milestone Aug 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Difficulty: Easy Someone with little to no experience in TSLint should be able to send a pull request for this issue. Status: Accepting PRs Type: Breaking Change Type: Bug
Projects
None yet
Development

No branches or pull requests

2 participants