Skip to content

Commit

Permalink
Refactor isort directive skips to use iterators (#5623)
Browse files Browse the repository at this point in the history
## Summary

We're doing some unsafe accesses to advance these iterators. It's easier
to model these as actual iterators to ensure safety everywhere. Also
added some additional test cases.

Closes #5621.
  • Loading branch information
charliermarsh authored Jul 8, 2023
1 parent 456273a commit 38fa305
Show file tree
Hide file tree
Showing 5 changed files with 109 additions and 30 deletions.
6 changes: 6 additions & 0 deletions crates/ruff/resources/test/fixtures/isort/skip.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,9 @@ def f():
import os # isort:skip
import collections
import abc


def f():
import sys; import os # isort:skip
import sys; import os # isort:skip # isort:skip
import sys; import os
10 changes: 10 additions & 0 deletions crates/ruff/resources/test/fixtures/isort/split.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,13 @@

import D
import B


import e
import f

# isort: split
# isort: split

import d
import c
76 changes: 46 additions & 30 deletions crates/ruff/src/rules/isort/block.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
use ruff_text_size::{TextRange, TextSize};
use rustpython_parser::ast::{self, ExceptHandler, MatchCase, Ranged, Stmt};
use std::iter::Peekable;
use std::slice;

use ruff_python_ast::source_code::Locator;
use ruff_python_ast::statement_visitor::StatementVisitor;
Expand Down Expand Up @@ -30,8 +32,8 @@ pub(crate) struct BlockBuilder<'a> {
locator: &'a Locator<'a>,
is_stub: bool,
blocks: Vec<Block<'a>>,
splits: &'a [TextSize],
cell_offsets: Option<&'a [TextSize]>,
splits: Peekable<slice::Iter<'a, TextSize>>,
cell_offsets: Option<Peekable<slice::Iter<'a, TextSize>>>,
exclusions: &'a [TextRange],
nested: bool,
}
Expand All @@ -47,12 +49,13 @@ impl<'a> BlockBuilder<'a> {
locator,
is_stub,
blocks: vec![Block::default()],
splits: &directives.splits,
splits: directives.splits.iter().peekable(),
exclusions: &directives.exclusions,
nested: false,
cell_offsets: source_kind
.and_then(SourceKind::notebook)
.map(Notebook::cell_offsets),
.map(Notebook::cell_offsets)
.map(|offsets| offsets.iter().peekable()),
}
}

Expand Down Expand Up @@ -126,45 +129,58 @@ where
'b: 'a,
{
fn visit_stmt(&mut self, stmt: &'b Stmt) {
// Track manual splits.
for (index, split) in self.splits.iter().enumerate() {
if stmt.start() >= *split {
self.finalize(self.trailer_for(stmt));
self.splits = &self.splits[index + 1..];
} else {
break;
// Track manual splits (e.g., `# isort: split`).
if self
.splits
.next_if(|split| stmt.start() >= **split)
.is_some()
{
// Skip any other splits that occur before the current statement, to support, e.g.:
// ```python
// # isort: split
// # isort: split
// import foo
// ```
while self
.splits
.peek()
.map_or(false, |split| stmt.start() >= **split)
{
self.splits.next();
}

self.finalize(self.trailer_for(stmt));
}

// Track Jupyter notebook cell offsets as splits. This will make sure
// that each cell is considered as an individual block to organize the
// imports in. Thus, not creating an edit which spans across multiple
// cells.
if let Some(cell_offsets) = self.cell_offsets {
for (index, split) in cell_offsets.iter().enumerate() {
if stmt.start() >= *split {
// We don't want any extra newlines between cells.
self.finalize(None);
self.cell_offsets = Some(&cell_offsets[index + 1..]);
} else {
break;
if let Some(cell_offsets) = self.cell_offsets.as_mut() {
if cell_offsets
.next_if(|cell_offset| stmt.start() >= **cell_offset)
.is_some()
{
// Skip any other cell offsets that occur before the current statement (e.g., in
// the case of multiple empty cells).
while cell_offsets
.peek()
.map_or(false, |split| stmt.start() >= **split)
{
cell_offsets.next();
}
}
}

// Test if the statement is in an excluded range
let mut is_excluded = false;
for (index, exclusion) in self.exclusions.iter().enumerate() {
if exclusion.end() < stmt.start() {
self.exclusions = &self.exclusions[index + 1..];
} else {
is_excluded = exclusion.contains(stmt.start());
break;
self.finalize(None);
}
}

// Track imports.
if matches!(stmt, Stmt::Import(_) | Stmt::ImportFrom(_)) && !is_excluded {
if matches!(stmt, Stmt::Import(_) | Stmt::ImportFrom(_))
&& !self
.exclusions
.iter()
.any(|exclusion| exclusion.contains(stmt.start()))
{
self.track_import(stmt);
} else {
self.finalize(self.trailer_for(stmt));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ skip.py:27:1: I001 [*] Import block is un-sorted or un-formatted
26 | import os # isort:skip
27 | / import collections
28 | | import abc
29 | |
| |_^ I001
30 |
31 | def f():
|
= help: Organize imports

Expand All @@ -41,5 +45,24 @@ skip.py:27:1: I001 [*] Import block is un-sorted or un-formatted
27 |+ import abc
27 28 | import collections
28 |- import abc
29 29 |
30 30 |
31 31 | def f():

skip.py:34:1: I001 [*] Import block is un-sorted or un-formatted
|
32 | import sys; import os # isort:skip
33 | import sys; import os # isort:skip # isort:skip
34 | / import sys; import os
|
= help: Organize imports

Fix
31 31 | def f():
32 32 | import sys; import os # isort:skip
33 33 | import sys; import os # isort:skip # isort:skip
34 |- import sys; import os
34 |+ import os
35 |+ import sys


Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ split.py:20:1: I001 [*] Import block is un-sorted or un-formatted
19 |
20 | / import D
21 | | import B
22 | |
| |_^ I001
23 |
24 | import e
|
= help: Organize imports

Expand All @@ -39,5 +43,25 @@ split.py:20:1: I001 [*] Import block is un-sorted or un-formatted
20 |+ import B
20 21 | import D
21 |- import B
22 22 |
23 23 |
24 24 | import e

split.py:30:1: I001 [*] Import block is un-sorted or un-formatted
|
28 | # isort: split
29 |
30 | / import d
31 | | import c
|
= help: Organize imports

Fix
27 27 | # isort: split
28 28 | # isort: split
29 29 |
30 |+import c
30 31 | import d
31 |-import c


0 comments on commit 38fa305

Please sign in to comment.