[`I001`] fix isort check for files with tabs and no indented blocks #2374

sciyoshi · 2023-01-31T02:47:58Z

This is a followup to #2361. The isort check still had an issue in a rather specific case: files with a multiline import, indented with tabs, and not containing any indented blocks.

The root cause is this: Stylist's indentation detection works by finding Indent tokens to determine the type of indentation used by a file. This works for indented code blocks (loops/classes/functions/etc) but does not work for multiline values, so falls back to 4 spaces if the file doesn't contain code blocks.

I considered a few possible solutions:

Fix detect_indentation to avoid tokenizing and instead use some other heuristic to determine indentation. This would have the benefit of working in other places where this is potentially an issue, but would still fail if the file doesn't contain any indentation at all, and would need to fall back to option 2 anyways.
Add an option for specifying the default indentation in Ruff's config. I think this would confusing, since it wouldn't affect the detection behavior and only operate as a fallback, has no other current application and would probably end up being overloaded for other things.
Relax the isort check by comparing the expected and actual code's lexed tokens. This would require an additional lexing step.
Relax the isort check by comparing the expected and actual code modulo whitespace at the start of lines.

This PR does approach 4, which in addition to being the simplest option, has the (expected, although I didn't benchmark) added benefit of improved performance, since the check no longer needs to do two allocations for the two dedent calls. I also believe that the check is still correct enough for all practical purposes.

sciyoshi · 2023-01-31T02:49:21Z

src/rules/isort/rules/organize_imports.rs

@@ -43,6 +44,17 @@ fn extract_indentation_range(body: &[&Stmt]) -> Range {
    Range::new(Location::new(location.row(), 0), location)
 }

+/// Compares two strings, returning true if they are equal modulo whitespace
+/// at the start of each line.
+fn matches_ignoring_indentation(val1: &str, val2: &str) -> bool {


This helper might be useful elsewhere, although I wasn't sure what the best place for it would be.

sciyoshi · 2023-01-31T02:56:10Z

A contrived example of code that will now pass the isort check but didn't before:

from module import (
 a,
  b,
   c,
    d,
)

I think this is acceptable and probably better handled by an autoformatter anyways.

charliermarsh · 2023-01-31T03:20:39Z

src/rules/isort/rules/organize_imports.rs

@@ -112,8 +124,8 @@ pub fn organize_imports(
        Location::new(range.location.row(), 0),
        Location::new(range.end_location.row() + 1 + num_trailing_lines, 0),
    );
-    let actual = dedent(locator.slice_source_code_range(&range));
-    if actual == dedent(&expected) {


(IIRC, we used to compare indent(&expected, indentation) to actual, but indent and dedent don't respect stylist.line_ending, and so files with CLRF newlines would always fail the comparison.)

charliermarsh · 2023-01-31T03:22:10Z

I agree with the reasoning you've prevented here. In theory, could we also check if actual has consistent indentation? (Assuming that, if the indentation is inconsistent, it should always be replaced?)

sciyoshi · 2023-01-31T05:46:48Z

In theory, could we also check if actual has consistent indentation?

If I'm understanding correctly, yes - definitely possible. Here's a change that implements that - although it does require the dedent call and performs allocations for getting prefixes (although there's probably a zero-allocation way to do that.) I'm not convinced it's necessary, but I can definitely include that in this PR if you'd like.

charliermarsh · 2023-01-31T12:19:50Z

Let's run with this. Thank you for fixing.

As another optional tweak to the heuristic: could we only ignore whitespace if we can't infer any indentation? That is, if we're falling back to the default?

sciyoshi · 2023-01-31T20:37:28Z

@charliermarsh thanks! And yes, that's possible as well. You can see the changes for that here - I can open a PR if you think that's a worthwhile improvement.

[I001] fix isort check for files with tabs and no indented blocks

0c48892

sciyoshi commented Jan 31, 2023

View reviewed changes

charliermarsh self-requested a review January 31, 2023 03:02

charliermarsh added the isort Related to import sorting label Jan 31, 2023

charliermarsh reviewed Jan 31, 2023

View reviewed changes

charliermarsh merged commit 09d593b into astral-sh:main Jan 31, 2023

sciyoshi deleted the isort-tabs branch January 31, 2023 20:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[`I001`] fix isort check for files with tabs and no indented blocks #2374

[`I001`] fix isort check for files with tabs and no indented blocks #2374

sciyoshi commented Jan 31, 2023 •

edited

Loading

sciyoshi Jan 31, 2023

sciyoshi commented Jan 31, 2023

charliermarsh Jan 31, 2023

charliermarsh commented Jan 31, 2023

sciyoshi commented Jan 31, 2023

charliermarsh commented Jan 31, 2023

sciyoshi commented Jan 31, 2023

[I001] fix isort check for files with tabs and no indented blocks #2374

[I001] fix isort check for files with tabs and no indented blocks #2374

Conversation

sciyoshi commented Jan 31, 2023 • edited Loading

sciyoshi Jan 31, 2023

Choose a reason for hiding this comment

sciyoshi commented Jan 31, 2023

charliermarsh Jan 31, 2023

Choose a reason for hiding this comment

charliermarsh commented Jan 31, 2023

sciyoshi commented Jan 31, 2023

charliermarsh commented Jan 31, 2023

sciyoshi commented Jan 31, 2023

[`I001`] fix isort check for files with tabs and no indented blocks #2374

[`I001`] fix isort check for files with tabs and no indented blocks #2374

sciyoshi commented Jan 31, 2023 •

edited

Loading