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

Update: max-lines reporting loc improvement (refs #12334) #13318

Merged
merged 6 commits into from Jun 19, 2020

Conversation

anikethsaha
Copy link
Member

@anikethsaha anikethsaha commented May 18, 2020

Prerequisites checklist

  • I have read the contributing guidelines.
  • The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[X] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Currently max-lines reports 1:1 for any numbers of line.
In this change, it will report the extra lines which are causing the error.

Example :

image

It will report from (loc.start) 2 and till (loc.end) 3 (total lines)

Is there anything you'd like reviewers to focus on?

refs #12334


This can be a bit more red lines as it will goes till eof.

One other option is that to report only the line from where the rest lines are extra. like in this case it would be the Line 3.

Let me know which one to keep.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label May 18, 2020
@anikethsaha anikethsaha changed the title Update: max-lines reporting loc refactore (refs #12334) Update: max-lines reporting loc improvement (refs #12334) May 18, 2020
@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels May 18, 2020
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a comment about what should be fixed if we decide to highlight the extra lines (or just the first extra line), but it could be fairly complex to do this correctly and I'm not sure if reporting the first offending line provides enough value to justify the added complexity.

lib/rules/max-lines.js Outdated Show resolved Hide resolved
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed the fact that we're already filtering out ignored lines instead of just counting them, so it turns out that this doesn't add much complexity.

Also, I'm still not sure that we want to find and report the first line that goes above the limit since we're not doing that in other similar rules (e.g., max-lines-per-function), but in the case that we do that, I left some notes about what should be changed in the actual iteration.

lib/rules/max-lines.js Outdated Show resolved Hide resolved
lib/rules/max-lines.js Outdated Show resolved Hide resolved
@anikethsaha
Copy link
Member Author

update :

image

lib/rules/max-lines.js Outdated Show resolved Hide resolved
@kaicataldo
Copy link
Member

@mdjermanovic Do you mind taking one more look?

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good! I have just a couple of small requests about tests.

tests/lib/rules/max-lines.js Outdated Show resolved Hide resolved
tests/lib/rules/max-lines.js Outdated Show resolved Hide resolved
tests/lib/rules/max-lines.js Outdated Show resolved Hide resolved
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@btmills btmills merged commit 09cc0a2 into eslint:master Jun 19, 2020
@anikethsaha anikethsaha deleted the fixes-12334-max-lines branch June 19, 2020 15:41
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Dec 17, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants