New: max-lines rule (fixes #6078)
#6321
Conversation
|
LGTM |
|
By analyzing the blame information on this pull request, we identified @nzakas, @scriptdaemon and @pedrottimark to be potential reviewers |
|
|
||
| This rule has a number or object option: | ||
|
|
||
| * `"max"` (default `100`) enforces a maximum number of lines in a file |
There was a problem hiding this comment.
The default should be 300: https://github.com/eslint/eslint/pull/6321/files#diff-662109f5bdfe0d12503c019cf4ae3ed2R52
There was a problem hiding this comment.
Thanks @TimvdLippe good catch. I intended to leave it as it is in JSCS (100) when I noticed it was different from what was proposed, but forgot to update the code.
@mysticatea why did you change it from 100 to 300? @eslint/eslint-team any opinions on what value this should be?
There was a problem hiding this comment.
Keep it at the same default as JSCS for maximum compatibility.
There was a problem hiding this comment.
That's an interesting question for larger discussion-- does JSCS compatibility mean match JSCS as closely as possible to make integration require less effort, or is ESLint allowed to have a different opinion on defaults and "JSCS compatibility" merely means there is a way to run your old JSCS settings in ESLint (even with different/non-default configuration)?
There was a problem hiding this comment.
I think matching JSCS is the way to go.
There was a problem hiding this comment.
I'm sorry, I'm not sure why I changed it. No reason.
There was a problem hiding this comment.
There is no "default" for this rule in JSCS, what mentioned in the docs is just an example.
100 feels weird to me, i'd say http://www.mind2b.com/component/content/article/24-software-module-size-and-file-size is interesting, my vote for default (if one should exist) is 500
There was a problem hiding this comment.
Oh, you are right, sorry for misleading here. We do have defaults for every rule, so yes, we have to choose one.
I'd go with something lower, the 300 lines proposed by @mysticatea seems like the sweet spot according to the article you linked. :D
|
Overall looks good, just need to update the default and the docs. |
|
LGTM |
|
LGTM |
|
PR updated addressing comments above. |
|
LGTM |
|
Default value updated to 300. |
Perhaps we can add some sort of explanation why we choose this number? |
|
Agree with @markelog, I think a little note and a reference to that link would go a long way. |
|
LGTM |
|
LGTM |
|
LGTM |
|
Lgtm |
eslint-deprecated
bot
added
the
archived due to age
No description provided.