[pycodestyle] Add too many blank lines (E303)
#4635
Conversation
crates/ruff/src/rules/pycodestyle/rules/too_many_blank_lines.rs
Outdated
Show resolved
Hide resolved
| let range = locator.full_lines_range(TextRange::new(line.start(), last_blank_line)); | ||
| let mut diagnostic = Diagnostic::new(TooManyBlankLines(nb_blank_lines), range); | ||
| diagnostic.set_fix(Fix::suggested(Edit::range_replacement( | ||
| "\n\n".to_string(), |
There was a problem hiding this comment.
We should use stylist (should be on checker) to use the user's preferred newline character.
| && !locator | ||
| .line(TextSize::new(line.start().to_u32() - 1)) | ||
| .trim() | ||
| .is_empty() |
There was a problem hiding this comment.
Determining the line is fairly expensive using. You may also be unlucky if the previous line ends with \r\n because you then index right between the \r and \n and there's no good way for the locator to know that it is now between a \r\n.
I'm not quite sure what the best way is to implement this rule. Ideally, it could keep a state between each line, counting the empty lines to this point (CC: @charliermarsh).
There was a problem hiding this comment.
I believe pycodestyle tracks the number of empty lines while building up logical lines -- we should probably do the same? That's now straightforward because we emit NonLogicalNewline whenever we see an empty line.
There was a problem hiding this comment.
We do, but the physical lines rule operates on the String and I think the logical lines check intentionally skips empty lines... Where would you place this rule? Would this be something new? A rule that directly works on the tokens.
There was a problem hiding this comment.
You'd place it at the next logical line -- you'd check the number of preceding empty lines, I think?
There was a problem hiding this comment.
Looking at the pycodestyle rule again, my implementation is wrong anyway. The following code should generate an error:
class MyClass(object):
def func1():
pass
def func2():
pass-> E303 too many blank lines (2)
I'll try again using logical lines.
There was a problem hiding this comment.
@MichaReiser @charliermarsh I've had another go using logical lines. It's a very, very rough draft (that might have broken a few other rules), but could you tell me if you think it's worth continuing on that path ?
The modifications I made to the logical_lines.rs file can be seen here, and the checking function is here. I've also had to remove the explicit skip of empty lines here.
I still haven't solved the issue with \r\n, but maybe I could count the number of characters to remove along with the number of blank lines ?
| previous_line_end = locator.full_line_end(previous_line_end); | ||
| let previous_line = locator.line(previous_line_end); |
There was a problem hiding this comment.
Line will have to repeat the search for the start and end of the line, which is fairly expensive.
Is there a way that we would only need to find the end (or start) and can then construct the TextRange ourselves (because we know what e.g. the end of the current line is?).
PR Check ResultsBenchmarkLinuxWindows |
| /// ## Example | ||
| /// ```python | ||
| /// def func1(): | ||
| /// pass | ||
| /// | ||
| /// | ||
| /// | ||
| /// def func2(): | ||
| /// pass | ||
| /// ``` |
There was a problem hiding this comment.
Because this issue is fixed by black, the rule name should be added to the KNOWN_FORMATTING_VIOLATIONS list to stop CI flagging that it should be fixed.
There was a problem hiding this comment.
Is there a list somewhere with black's rules names ?
There was a problem hiding this comment.
In this case, you would add too_many_blank_lines to the list as KNOWN_FORMATTING_VIOLATIONS relates to Ruff's rules rather than black.
|
I'm closing this PR since I'm redoing it using logical lines instead of physical lines. |
Summary
This PR is part of #2402, it adds the
E303error rule with its fix.Test Plan
It was tested on the added fixture:
crates/ruff/resources/test/fixtures/pycodestyle/E303.py.