Skip to content

Commit

Permalink
Always place non-relative imports after relative imports (#10669)
Browse files Browse the repository at this point in the history
## Summary

When `relative-imports-order = "closest-to-furthest"` is set, we should
_still_ put non-relative imports after relative imports. It's rare for
them to be in the same section, but _possible_ if you use
`known-local-folder`.

Closes #10655.

## Test Plan

New tests.

Also sorted this file:

```python
from ..models import ABC
from .models import Question
from .utils import create_question
from django_polls.apps.polls.models import Choice
```

With both:

- `isort view.py`
- `ruff check view.py --select I --fix`

And the following `pyproject.toml`:

```toml
[tool.ruff.lint.isort]
order-by-type = false
relative-imports-order = "closest-to-furthest"
known-local-folder = ["django_polls"]

[tool.isort]
profile = "black"
reverse_relative = true
known_local_folder = ["django_polls"]
```

I verified that Ruff and isort gave the same result, and that they
_still_ gave the same result when removing the relevant setting:

```toml
[tool.ruff.lint.isort]
order-by-type = false
known-local-folder = ["django_polls"]

[tool.isort]
profile = "black"
known_local_folder = ["django_polls"]
```
  • Loading branch information
charliermarsh committed Mar 30, 2024
1 parent fc54f53 commit 75e0142
Show file tree
Hide file tree
Showing 6 changed files with 86 additions and 21 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,5 @@
import leading_prefix
import os
from . import leading_prefix
from .. import trailing_prefix
from ruff import check
25 changes: 25 additions & 0 deletions crates/ruff_linter/src/rules/isort/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,31 @@ mod tests {
Ok(())
}

#[test_case(Path::new("separate_local_folder_imports.py"))]
fn known_local_folder_closest(path: &Path) -> Result<()> {
let snapshot = format!("known_local_folder_closest_{}", path.to_string_lossy());
let diagnostics = test_path(
Path::new("isort").join(path).as_path(),
&LinterSettings {
isort: super::settings::Settings {
known_modules: KnownModules::new(
vec![],
vec![],
vec![pattern("ruff")],
vec![],
FxHashMap::default(),
),
relative_imports_order: RelativeImportsOrder::ClosestToFurthest,
..super::settings::Settings::default()
},
src: vec![test_resource_path("fixtures/isort")],
..LinterSettings::for_rule(Rule::UnsortedImports)
},
)?;
assert_messages!(snapshot, diagnostics);
Ok(())
}

#[test_case(Path::new("case_sensitive.py"))]
fn case_sensitive(path: &Path) -> Result<()> {
let snapshot = format!("case_sensitive_{}", path.to_string_lossy());
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
---
source: crates/ruff_linter/src/rules/isort/mod.rs
---
separate_local_folder_imports.py:1:1: I001 [*] Import block is un-sorted or un-formatted
|
1 | / import sys
2 | | import ruff
3 | | import leading_prefix
4 | | import os
5 | | from . import leading_prefix
6 | | from .. import trailing_prefix
7 | | from ruff import check
|
= help: Organize imports

Safe fix
1 |+import os
1 2 | import sys
3 |+
4 |+import leading_prefix
5 |+
2 6 | import ruff
3 |-import leading_prefix
4 |-import os
5 7 | from . import leading_prefix
6 8 | from .. import trailing_prefix
7 9 | from ruff import check
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ separate_local_folder_imports.py:1:1: I001 [*] Import block is un-sorted or un-f
3 | | import leading_prefix
4 | | import os
5 | | from . import leading_prefix
6 | | from .. import trailing_prefix
7 | | from ruff import check
|
= help: Organize imports

Expand All @@ -20,6 +22,7 @@ separate_local_folder_imports.py:1:1: I001 [*] Import block is un-sorted or un-f
2 6 | import ruff
3 |-import leading_prefix
4 |-import os
5 7 | from . import leading_prefix


7 |+from .. import trailing_prefix
5 8 | from . import leading_prefix
6 |-from .. import trailing_prefix
7 9 | from ruff import check
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,22 @@ separate_local_folder_imports.py:1:1: I001 [*] Import block is un-sorted or un-f
3 | | import leading_prefix
4 | | import os
5 | | from . import leading_prefix
6 | | from .. import trailing_prefix
7 | | from ruff import check
|
= help: Organize imports

Safe fix
1 |+import os
1 2 | import sys
3 |+
2 4 | import ruff
5 |+
3 6 | import leading_prefix
4 |-import os
7 |+
5 8 | from . import leading_prefix


1 |+import os
1 2 | import sys
3 |+
2 4 | import ruff
5 |+from ruff import check
6 |+
3 7 | import leading_prefix
4 |-import os
8 |+
9 |+from .. import trailing_prefix
5 10 | from . import leading_prefix
6 |-from .. import trailing_prefix
7 |-from ruff import check
18 changes: 11 additions & 7 deletions crates/ruff_linter/src/rules/isort/sorting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ impl<'a> From<String> for NatOrdStr<'a> {
pub(crate) enum Distance {
Nearest(u32),
Furthest(Reverse<u32>),
None,
}

#[derive(Debug, Copy, Clone, PartialOrd, Ord, PartialEq, Eq)]
Expand Down Expand Up @@ -101,17 +102,20 @@ impl<'a> ModuleKey<'a> {
style: ImportStyle,
settings: &Settings,
) -> Self {
let level = level.unwrap_or_default();

let force_to_top = !name.is_some_and(|name| settings.force_to_top.contains(name)); // `false` < `true` so we get forced to top first

let maybe_length = (settings.length_sort
|| (settings.length_sort_straight && style == ImportStyle::Straight))
.then_some(name.map(str::width).unwrap_or_default() + level as usize);

let distance = match settings.relative_imports_order {
RelativeImportsOrder::ClosestToFurthest => Distance::Nearest(level),
RelativeImportsOrder::FurthestToClosest => Distance::Furthest(Reverse(level)),
.then_some(
name.map(str::width).unwrap_or_default() + level.unwrap_or_default() as usize,
);

let distance = match level {
None | Some(0) => Distance::None,
Some(level) => match settings.relative_imports_order {
RelativeImportsOrder::ClosestToFurthest => Distance::Nearest(level),
RelativeImportsOrder::FurthestToClosest => Distance::Furthest(Reverse(level)),
},
};

let maybe_lowercase_name = name.and_then(|name| {
Expand Down

0 comments on commit 75e0142

Please sign in to comment.