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

[pycodestyle] Avoid blank line rules for the first logical line in cell #10291

Merged
merged 16 commits into from
Mar 25, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
180 changes: 180 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/pycodestyle/E30.ipynb
Original file line number Diff line number Diff line change
@@ -0,0 +1,180 @@
{
"cells": [
{
"cell_type": "code",
"execution_count": null,
"metadata": {
"id": "palRUQyD-U6u"
},
"outputs": [],
"source": [
"some_string = \"123123\""
]
},
{
"cell_type": "code",
"execution_count": null,
"metadata": {
"id": "UWdDLRyf-Zz0"
},
"outputs": [],
"source": [
"some_computation = 1 + 1"
]
},
{
"cell_type": "code",
"execution_count": null,
"metadata": {
"id": "YreT1sTr-c32"
},
"outputs": [],
"source": [
"some_computation"
]
},
{
"cell_type": "code",
"execution_count": null,
"metadata": {
"id": "V48ppml7-h0f"
},
"outputs": [],
"source": [
"def fn():\n",
" print(\"Hey!\")"
]
},
{
"cell_type": "code",
"execution_count": null,
"metadata": {
"id": "cscw_8Xv-lYQ"
},
"outputs": [],
"source": [
"fn()"
]
},
{
"cell_type": "code",
"execution_count": null,
"metadata": {},
"outputs": [],
"source": [
"# E301\n",
"class Class:\n",
" \"\"\"Class for minimal repo.\"\"\"\n",
"\n",
" def method(cls) -> None:\n",
" pass\n",
" @classmethod\n",
" def cls_method(cls) -> None:\n",
" pass\n",
"# end"
]
},
{
"cell_type": "code",
"execution_count": null,
"metadata": {},
"outputs": [],
"source": [
"# E302\n",
"def a():\n",
" pass\n",
"\n",
"def b():\n",
" pass\n",
"# end"
]
},
{
"cell_type": "code",
"execution_count": null,
"metadata": {},
"outputs": [],
"source": [
"# E303\n",
"def fn():\n",
" _ = None\n",
"\n",
"\n",
" # arbitrary comment\n",
"\n",
" def inner(): # E306 not expected (pycodestyle detects E306)\n",
" pass\n",
"# end"
]
},
{
"cell_type": "code",
"execution_count": null,
"metadata": {},
"outputs": [],
"source": [
"# E304\n",
"@decorator\n",
"\n",
"def function():\n",
" pass\n",
"# end"
]
},
{
"cell_type": "code",
"execution_count": null,
"metadata": {},
"outputs": [],
"source": [
"# E305:7:1\n",
"def fn():\n",
" print()\n",
"\n",
" # comment\n",
"\n",
" # another comment\n",
"fn()\n",
"# end"
]
},
{
"cell_type": "code",
"execution_count": null,
"metadata": {},
"outputs": [],
"source": [
"# E306:3:5\n",
"def a():\n",
" x = 1\n",
" def b():\n",
" pass\n",
"# end"
]
}
],
"metadata": {
"colab": {
"provenance": []
},
"kernelspec": {
"display_name": "Python 3 (ipykernel)",
"language": "python",
"name": "python3"
},
"language_info": {
"codemirror_mode": {
"name": "ipython",
"version": 3
},
"file_extension": ".py",
"mimetype": "text/x-python",
"name": "python",
"nbconvert_exporter": "python",
"pygments_lexer": "ipython3",
"version": "3.11.7"
}
},
"nbformat": 4,
"nbformat_minor": 4
}
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/checkers/tokens.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ pub(crate) fn check_tokens(
Rule::BlankLinesAfterFunctionOrClass,
Rule::BlankLinesBeforeNestedDefinition,
]) {
BlankLinesChecker::new(locator, stylist, settings, source_type)
BlankLinesChecker::new(locator, stylist, settings, source_type, cell_offsets)
.check_lines(tokens, &mut diagnostics);
}

Expand Down
16 changes: 16 additions & 0 deletions crates/ruff_linter/src/rules/pycodestyle/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,22 @@ mod tests {
Ok(())
}

#[test_case(Rule::BlankLineBetweenMethods)]
#[test_case(Rule::BlankLinesTopLevel)]
#[test_case(Rule::TooManyBlankLines)]
#[test_case(Rule::BlankLineAfterDecorator)]
#[test_case(Rule::BlankLinesAfterFunctionOrClass)]
#[test_case(Rule::BlankLinesBeforeNestedDefinition)]
fn blank_lines_notebook(rule_code: Rule) -> Result<()> {
let snapshot = format!("blank_lines_{}_notebook", rule_code.noqa_code());
let diagnostics = test_path(
Path::new("pycodestyle").join("E30.ipynb"),
&settings::LinterSettings::for_rule(rule_code),
)?;
assert_messages!(snapshot, diagnostics);
Ok(())
}

#[test]
fn blank_lines_typing_stub_isort() -> Result<()> {
let diagnostics = test_path(
Expand Down
32 changes: 30 additions & 2 deletions crates/ruff_linter/src/rules/pycodestyle/rules/blank_lines.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use itertools::Itertools;
use ruff_notebook::CellOffsets;
use std::cmp::Ordering;
use std::num::NonZeroU32;
use std::slice::Iter;
Expand Down Expand Up @@ -351,6 +352,9 @@ struct LogicalLineInfo {
// `true` if this is not a blank but only consists of a comment.
is_comment_only: bool,

/// If running on a notebook, whether the line is the first non-trivia line of its cell.
is_cell_first_non_comment_line: bool,

/// `true` if the line is a string only (including trivia tokens) line, which is a docstring if coming right after a class/function definition.
is_docstring: bool,

Expand Down Expand Up @@ -379,20 +383,27 @@ struct LinePreprocessor<'a> {
/// Maximum number of consecutive blank lines between the current line and the previous non-comment logical line.
/// One of its main uses is to allow a comment to directly precede a class/function definition.
max_preceding_blank_lines: BlankLines,
/// The cell offsets of the notebook (if running on a notebook).
cell_offsets: Option<&'a CellOffsets>,
/// If running on a notebook, whether the line is the first non-trivia line of its cell.
is_cell_first_non_comment_line: bool,
}

impl<'a> LinePreprocessor<'a> {
fn new(
tokens: &'a [LexResult],
locator: &'a Locator,
indent_width: IndentWidth,
cell_offsets: Option<&'a CellOffsets>,
) -> LinePreprocessor<'a> {
LinePreprocessor {
tokens: tokens.iter(),
locator,
line_start: TextSize::new(0),
max_preceding_blank_lines: BlankLines::Zero,
indent_width,
is_cell_first_non_comment_line: cell_offsets.is_some(),
cell_offsets,
}
}
}
Expand Down Expand Up @@ -491,6 +502,7 @@ impl<'a> Iterator for LinePreprocessor<'a> {
last_token,
logical_line_end: range.end(),
is_comment_only: line_is_comment_only,
is_cell_first_non_comment_line: self.is_cell_first_non_comment_line,
is_docstring,
indent_length,
blank_lines,
Expand All @@ -505,6 +517,15 @@ impl<'a> Iterator for LinePreprocessor<'a> {
// Set the start for the next logical line.
self.line_start = range.end();

if self
.cell_offsets
.is_some_and(|cell_offsets| cell_offsets.contains(&self.line_start))
Copy link
Member

Choose a reason for hiding this comment

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

You may want to use containing_range here. contains is O(n), containing_range is O(log(n)). But I wonder if we could do better by keeping an iterator with the cell offsets (they're sorted) and peek at the first offset and:

  • while it is smaller than the line_start, call next
  • if it is not None, you know its inside of the cell.
    This gives you O(n) performance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MichaReiser It's not exactly what you said, but I gave it a try in 3f703af. What do you think ?

{
self.is_cell_first_non_comment_line = true;
} else if !line_is_comment_only {
self.is_cell_first_non_comment_line = false;
}

Copy link
Member

Choose a reason for hiding this comment

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

Is it okay that we set is_cell_first_non_comment_line to true even if line_is_comment_only is true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's fine since if a cell starts with a comment, we don't want to run the check on that comment (since it's the first line).
However the naming is pretty bad indeed, I'll try to improve it.

Edit: renamed in f29a53d.

return Some(logical_line);
}
_ => {}
Expand Down Expand Up @@ -654,6 +675,7 @@ pub(crate) struct BlankLinesChecker<'a> {
lines_after_imports: isize,
lines_between_types: usize,
source_type: PySourceType,
cell_offsets: Option<&'a CellOffsets>,
}

impl<'a> BlankLinesChecker<'a> {
Expand All @@ -662,6 +684,7 @@ impl<'a> BlankLinesChecker<'a> {
stylist: &'a Stylist<'a>,
settings: &crate::settings::LinterSettings,
source_type: PySourceType,
cell_offsets: Option<&'a CellOffsets>,
) -> BlankLinesChecker<'a> {
BlankLinesChecker {
stylist,
Expand All @@ -670,14 +693,16 @@ impl<'a> BlankLinesChecker<'a> {
lines_after_imports: settings.isort.lines_after_imports,
lines_between_types: settings.isort.lines_between_types,
source_type,
cell_offsets,
}
}

/// E301, E302, E303, E304, E305, E306
pub(crate) fn check_lines(&self, tokens: &[LexResult], diagnostics: &mut Vec<Diagnostic>) {
let mut prev_indent_length: Option<usize> = None;
let mut state = BlankLinesState::default();
let line_preprocessor = LinePreprocessor::new(tokens, self.locator, self.indent_width);
let line_preprocessor =
LinePreprocessor::new(tokens, self.locator, self.indent_width, self.cell_offsets);

for logical_line in line_preprocessor {
// Reset `follows` after a dedent:
Expand All @@ -696,7 +721,10 @@ impl<'a> BlankLinesChecker<'a> {
state.class_status.update(&logical_line);
state.fn_status.update(&logical_line);

if state.is_not_first_logical_line {
if state.is_not_first_logical_line
// Ignore the first line of each cell in notebooks.
&& !logical_line.is_cell_first_non_comment_line
{
self.check_line(&logical_line, &state, prev_indent_length, diagnostics);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
---
source: crates/ruff_linter/src/rules/pycodestyle/mod.rs
---
E30.ipynb:13:5: E301 [*] Expected 1 blank line, found 0
|
11 | def method(cls) -> None:
12 | pass
13 | @classmethod
| ^ E301
14 | def cls_method(cls) -> None:
15 | pass
|
= help: Add missing blank line

ℹ Safe fix
10 10 |
11 11 | def method(cls) -> None:
12 12 | pass
13 |+
13 14 | @classmethod
14 15 | def cls_method(cls) -> None:
15 16 | pass
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
---
source: crates/ruff_linter/src/rules/pycodestyle/mod.rs
---
E30.ipynb:21:1: E302 [*] Expected 2 blank lines, found 1
|
19 | pass
20 |
21 | def b():
| ^^^ E302
22 | pass
23 | # end
|
= help: Add missing blank line(s)

ℹ Safe fix
18 18 | def a():
19 19 | pass
20 20 |
21 |+
21 22 | def b():
22 23 | pass
23 24 | # end
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
source: crates/ruff_linter/src/rules/pycodestyle/mod.rs
---
E30.ipynb:29:5: E303 [*] Too many blank lines (2)
|
29 | # arbitrary comment
| ^^^^^^^^^^^^^^^^^^^ E303
30 |
31 | def inner(): # E306 not expected (pycodestyle detects E306)
|
= help: Remove extraneous blank line(s)

ℹ Safe fix
25 25 | def fn():
26 26 | _ = None
27 27 |
28 |-
29 28 | # arbitrary comment
30 29 |
31 30 | def inner(): # E306 not expected (pycodestyle detects E306)