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

remove lint rules for max-line #247

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

warm-coolguy
Copy link
Member

@warm-coolguy warm-coolguy commented Feb 24, 2025

Summary

I don't think the rules "max-lines" and "max-lines-per-function" should be kept and that they didn't stand the test of time.

  • Forces coders to split files/functions at an arbitrary LOC count
  • Splits may not have any semantic meaning or useful result
  • Splits may actually reduce readability since a lot of context forwarding may be required, adding unnecessary syntax
  • Give incentive for code golfing, decreasing readability
  • Give incentive for deleting empty lines of code, decreasing readability

Limits are especially irrelevant when we're operating in an environment where many lines of code may contain very easy to digest complexity like:

...
catch (e) {
  console.error(
    '@polar/source: An error occured when doing a thing.',
    e
  )
  dispatch(
    'plugin/toast/addToast',
    {
      type: 'warning',
      text: 'some.locale.key.that.usually.has.a.long.length',
      timeout: 10000,
    },
    { root: true }
  )
}

As illustrated, LOCs do not hold any meaning regarding complexity. I'm still all for keeping functions as short as possible and separating code by concerns, but the current state of linting blocks valid implementations. It's the wrong hammer for this screw, and I don't think there's an automatable solution to this.

@warm-coolguy warm-coolguy added the refactor Refactoring of previous code label Feb 24, 2025
@warm-coolguy warm-coolguy self-assigned this Feb 24, 2025
@dopenguin
Copy link
Member

In most cases, a restriction of lines of code is a stylistic choice.
Most plugins for eslint don't entforce it, as it's up to each developer to take care that a function doesn't get too complex.

In most cases, I'd agree with you: The rules for the max-length of a function often are not helpful and lead to refactoring steps that are mostly not required.

However, removing the rule altogether doesn't seem to be the solution here.
In my opinion, new developers should at least get a warning that a function may be too long. This way, they'd rethink if it is actually necessary to write a function that maybe has 300 LoC (which would be "fine" if we'd remove this) or think about refactoring before it gets to a review.

Also, I don't see any reasoning of removing the rule for a maximum length for a file. We can discuss when this rule applies (not sure if 300 is too low) but not if.

@warm-coolguy
Copy link
Member Author

What's the solution if someone wants the function/file to keep the length despite the warning? We can't just let the warning stand since large portions of a file may be underlined in most IDEs, which makes the code and other warnings indecipherable.

Will a simple @eslint-disable of the rule without further ado in the appropriate position suffice? If so, I can change this PR to reducing the severity level from "error" to "warn".

@dopenguin
Copy link
Member

What's the solution if someone wants the function/file to keep the length despite the warning? We can't just let the warning stand since large portions of a file may be underlined in most IDEs, which makes the code and other warnings indecipherable.

Will a simple @eslint-disable of the rule without further ado in the appropriate position suffice? If so, I can change this PR to reducing the severity level from "error" to "warn".

For files, I'd still go with "error". As mentioned before, we can discuss about the length that is allowed.

Regarding function lengths: IMO, the route we are currently taking should be the way. If a function is too long but it is certain that it is totally fine, then add a comment and disable the warning for that specific function.

This linting rule should inspire a developer to hold still and think if the design of the written function can stay as is or is a function that would be refactored given the next time someones comes near it.
Otherwise, it's generally up to a maintainer during code review to point a dev towards the length of the function and keeping the rule for function length helps during reviews.

@warm-coolguy
Copy link
Member Author

warm-coolguy commented Feb 24, 2025

What's the solution if someone wants the function/file to keep the length despite the warning? We can't just let the warning stand since large portions of a file may be underlined in most IDEs, which makes the code and other warnings indecipherable.
Will a simple @eslint-disable of the rule without further ado in the appropriate position suffice? If so, I can change this PR to reducing the severity level from "error" to "warn".

For files, I'd still go with "error". As mentioned before, we can discuss about the length that is allowed.

Regarding function lengths: IMO, the route we are currently taking should be the way. If a function is too long but it is certain that it is totally fine, then add a comment and disable the warning for that specific function.

This linting rule should inspire a developer to hold still and think if the design of the written function can stay as is or is a function that would be refactored given the next time someones comes near it. Otherwise, it's generally up to a maintainer during code review to point a dev towards the length of the function and keeping the rule for function length helps during reviews.

  • "max-lines-per-function": That'd literally be no change at all since a warning would still require coders to comment the position with both a disabler and reasoning, so it's an error in warning's clothing that needs to be adressed this way or another, but is harder to find. Now that I think of it, I'm actually sure again that we should simply remove the rule due to this hassle.
  • "max-lines" should go, too, for the same reason. No hard border will resolve the problems in principle, and there'll always be files overstepping the limit by 1 line for some good reason, no matter where you put it, leading to the problems mentioned in the OP.

There's just no LOC-based rules to make anyone work in a sensible fashion, and it may easily produce the opposite.

Since we two are probably stuck now, I'll address this topic in the next meeting where we can get majorities in voting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactoring of previous code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants