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
Conversation
There was a problem hiding this 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.
There was a problem hiding this 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.
@mdjermanovic Do you mind taking one more look? |
There was a problem hiding this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
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)
Currently
max-lines
reports1:1
for any numbers of line.In this change, it will report the extra lines which are causing the error.
Example :
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.