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

feat: report only lines that exceed the limit in max-lines-per-function #15140

Merged
merged 13 commits into from Dec 2, 2021

Conversation

The-x-Theorist
Copy link
Contributor

Prerequisites checklist

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)

Updated the max-lines-per-function rule to report only the count of those lines which exceed the specified max-lines limit, instead of reporting total lines from the start of the function to the end.

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

nope

@eslint-github-bot eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label Oct 7, 2021
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 7, 2021

CLA Signed

The committers are authorized under a signed CLA.

@eslint-github-bot
Copy link

Hi @The-x-Theorist!, thanks for the Pull Request

The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The length of the commit message must be less than or equal to 72

Read more about contributing to ESLint here

@The-x-Theorist The-x-Theorist changed the title Update: report only the exceeded lines in max-lines-per-function (fixes #15098) Update: report only exceeded lines in max-lines-per-function (fixes #15098) Oct 7, 2021
@eslint-github-bot
Copy link

Hi @The-x-Theorist!, thanks for the Pull Request

The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The length of the commit message must be less than or equal to 72

Read more about contributing to ESLint here

@The-x-Theorist The-x-Theorist changed the title Update: report only exceeded lines in max-lines-per-function (fixes #15098) Update: report the exceeded lines in max-lines-per-function (fixes #15098) Oct 7, 2021
@eslint-github-bot
Copy link

Hi @The-x-Theorist!, thanks for the Pull Request

The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The length of the commit message must be less than or equal to 72

Read more about contributing to ESLint here

@The-x-Theorist The-x-Theorist changed the title Update: report the exceeded lines in max-lines-per-function (fixes #15098) Update: reports exceeded lines in max-lines-per-function (fixes #15098) Oct 7, 2021
@eslint-github-bot
Copy link

Hi @The-x-Theorist!, thanks for the Pull Request

The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The length of the commit message must be less than or equal to 72

Read more about contributing to ESLint here

@The-x-Theorist The-x-Theorist changed the title Update: reports exceeded lines in max-lines-per-function (fixes #15098) Update: report exceeded lines in max-lines-per-function (fixes #15098) Oct 7, 2021
@eslint-github-bot

This comment has been minimized.

@The-x-Theorist The-x-Theorist changed the title Update: report exceeded lines in max-lines-per-function (fixes #15098) Update: reports only the exceeded lines (fixes #15098) Oct 7, 2021
@eslint-github-bot

This comment has been minimized.

@The-x-Theorist The-x-Theorist changed the title Update: reports only the exceeded lines (fixes #15098) Update: reports exceeded lines (fixes #15098) Oct 7, 2021
@eslint-github-bot

This comment has been minimized.

Copy link
Contributor

@snitin315 snitin315 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 changes look good.

lib/rules/max-lines-per-function.js Outdated Show resolved Hide resolved
@snitin315 snitin315 added accepted There is consensus among the team that this change meets the criteria for inclusion rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Oct 22, 2021
Co-authored-by: Nitin Kumar <snitin315@gmail.com>
@mdjermanovic mdjermanovic added the enhancement This change enhances an existing feature of ESLint label Oct 22, 2021
Copy link
Contributor Author

@The-x-Theorist The-x-Theorist left a comment

Choose a reason for hiding this comment

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

@mdjermanovic Please suggest a change for the correction you want to be resolved?

@mdjermanovic mdjermanovic changed the title Update: reports exceeded lines (fixes #15098) feat: reports exceeded lines (fixes #15098) Oct 26, 2021
@The-x-Theorist
Copy link
Contributor Author

Is this change works good with skipBlankLines and skipComments too? @mdjermanovic

@mdjermanovic
Copy link
Member

Is this change works good with skipBlankLines and skipComments too?

It doesn't look good:

image

It should highlight lines starting from the first that is above the max, of those that are not skipped:

image

@The-x-Theorist
Copy link
Contributor Author

@mdjermanovic Now, I have changed the way skipComments and skipBlankLines was handled for loc property.

@nzakas
Copy link
Member

nzakas commented Nov 25, 2021

@The-x-Theorist are you still working on this?

@The-x-Theorist
Copy link
Contributor Author

@nzakas No, I have completed the work it's ready to be merged.

lib/rules/max-lines-per-function.js Outdated Show resolved Hide resolved
lib/rules/max-lines-per-function.js Outdated Show resolved Hide resolved
lib/rules/max-lines-per-function.js Outdated Show resolved Hide resolved
lib/rules/max-lines-per-function.js Outdated Show resolved Hide resolved
lib/rules/max-lines-per-function.js Outdated Show resolved Hide resolved
The-x-Theorist and others added 5 commits November 26, 2021 11:48
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
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!

@mdjermanovic mdjermanovic changed the title feat: reports exceeded lines (fixes #15098) feat: report only lines that exceed the limit in max-lines-per-function Nov 28, 2021
Copy link
Member

@nzakas nzakas 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!

@nzakas nzakas merged commit 8f44cf5 into eslint:main Dec 2, 2021
@The-x-Theorist The-x-Theorist deleted the max-lines-per-function-rule-change branch December 2, 2021 06:16
@ljharb
Copy link
Sponsor Contributor

ljharb commented Dec 4, 2021

This seems like a bit of a breaking change.

It's causing my eslint-disable comments - that have always been positioned at the top of the function - to be considered unused, and instead the random line that happens to break the threshold is reported on.

mdjermanovic added a commit that referenced this pull request Dec 7, 2021
Revert "feat: report only lines that exceed the limit in max-lines-per-function (#15140)"

This reverts commit 8f44cf5.
mdjermanovic added a commit that referenced this pull request Dec 7, 2021
…15397)

Revert "feat: report only lines that exceed the limit in max-lines-per-function (#15140)"

This reverts commit 8f44cf5.
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jun 1, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jun 1, 2022
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.

Rule Change: Make max-lines-per-function only reports lines that exceed the limit, not all
5 participants