-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[pylint
] Implement too-many-lines
(PLC0302
)
#10262
Conversation
pylint
] implement too-many-lines
(PLC0302
)pylint
] Implement too-many-lines
(PLC0302
)
|
code | total | + violation | - violation | + fix | - fix |
---|---|---|---|---|---|
PLC0302 | 141 | 141 | 0 | 0 | 0 |
D100 | 4 | 2 | 2 | 0 | 0 |
Formatter (stable)
✅ ecosystem check detected no format changes.
Formatter (preview)
✅ ecosystem check detected no format changes.
Nevermind |
Would be great to have this merged 🙌 🙌 |
impl Violation for TooManyLines { | ||
#[derive_message_formats] | ||
fn message(&self) -> String { | ||
format!("Too many lines in module") |
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.
Could it also show the number? e.g. pylint shows Too many lines in module (3000/2000)
I think use >
sign is more proper there:
Too many lines in module (3000>2000)
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.
Done
let lines = locator.contents().universal_newlines(); | ||
let length = lines.count() + 1; | ||
|
||
if length > settings.pylint.max_module_lines { |
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.
let lines = locator.contents().universal_newlines(); | |
let length = lines.count() + 1; | |
if length > settings.pylint.max_module_lines { | |
let lines_count = locator.contents().universal_newlines().count(); | |
if lines_count > settings.pylint.max_module_lines { |
Is there any reason that you increased the lines count by one?
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.
universal_newlines splits on newline characters, so if the last line is empty it won't be counted. Really what I want to count is the number of newline characters, but I didn't find anything to do that.
It's a good point though that if the last line is not empty, then this will be off-by-one in the other direction.
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.
Correct, it should increase by one when the last line is not empty.
I think lines count of all below contents is 1:
first
first\n
first\r\n
In this PR, first\n
consumed as 2 lines, I'm not sure if it is expected behavior.
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'll check against pylint
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.
Ok pylint doesn't count the final newline, but it does count multiple final new lines, so first\n\n
should be 2 but first\n
should be one. I will update it.
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.
Done
477cf1a
to
b59d4b6
Compare
Thanks for your contribution. Unfortunately, the rule is incompatible with the formatter because the formatter can introduce new violations by splitting lines across multiple lines, making the module exceed the configured max lines. We currently want to hold off from adding new rules that conflict with the formatter because everyone using I'm sorry we didn't point this out on the PyLint issue before you started implementing it. This must feel frustrating to you and we could have done better. I'm trying to catch up with rules and plan to first go through the open PRs and later triage open issues to ensure we communicate clearly which rules will be accepted today. |
@MichaReiser No worries. I do hope we can merge this eventually though, because I think it is an important check. |
Summary
Implements the too-many-lines rule (C0302) from pylint.
Test Plan
New fixtures have been added
Part of #970