Skip to content

Commit

Permalink
fix(isort): comply with isort when sorting sub-package modules
Browse files Browse the repository at this point in the history
e.g. packages named "foo.bar"

additionally make sure `__future__` imports are always sorted at the
very top.

Issue #3059
  • Loading branch information
spaceone committed Feb 22, 2023
1 parent f0e0efc commit 594a8f5
Show file tree
Hide file tree
Showing 13 changed files with 169 additions and 25 deletions.
2 changes: 0 additions & 2 deletions crates/ruff/resources/test/fixtures/isort/pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,3 @@ line-length = 88
[tool.ruff.isort]
lines-after-imports = 3
lines-between-types = 2
known-local-folder = ["ruff"]
force-to-top = ["lib1", "lib3", "lib5", "lib3.lib4", "z"]
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,10 @@
import numpy as np
import os
from leading_prefix import Class
import baz
from foo import bar, baz
from foo.bar import blah, blub
from foo.bar.baz import something
import foo
import foo.bar
import foo.bar.baz
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@
import sys
import numpy as np
import os
import foo.bar
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ fn fix_banned_relative_import(
module_path: Option<&Vec<String>>,
stylist: &Stylist,
) -> Option<Fix> {
// Only fix is the module path is known.
// Only fix if the module path is known.
if let Some(mut parts) = module_path.cloned() {
// Remove relative level from module path.
for _ in 0..*level? {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,15 +150,13 @@ pub fn typing_only_runtime_import(
&& binding.runtime_usage.is_none()
&& binding.synthetic_usage.is_none()
{
// Extract the module base and level from the full name.
// Ex) `foo.bar.baz` -> `foo`, `0`
// Extract the module level from the full name.
// Ex) `.foo.bar.baz` -> `foo`, `1`
let module_base = full_name.split('.').next().unwrap();
let level = full_name.chars().take_while(|c| *c == '.').count();

// Categorize the import.
match categorize(
module_base,
full_name,
Some(&level),
&settings.src,
package,
Expand Down
25 changes: 17 additions & 8 deletions crates/ruff/src/rules/isort/categorize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ enum Reason<'a> {

#[allow(clippy::too_many_arguments)]
pub fn categorize(
module_base: &str,
module_name: &str,
level: Option<&usize>,
src: &[PathBuf],
package: Option<&Path>,
Expand All @@ -48,9 +48,20 @@ pub fn categorize(
extra_standard_library: &BTreeSet<String>,
target_version: PythonVersion,
) -> ImportType {
let module_base = module_name.split('.').next().unwrap();
let (import_type, reason) = {
if level.map_or(false, |level| *level > 0) {
(ImportType::LocalFolder, Reason::NonZeroLevel)
} else if module_base == "__future__" {
(ImportType::Future, Reason::Future)
} else if known_first_party.contains(module_name) {
(ImportType::FirstParty, Reason::KnownFirstParty)
} else if known_third_party.contains(module_name) {
(ImportType::ThirdParty, Reason::KnownThirdParty)
} else if known_local_folder.contains(module_name) {
(ImportType::LocalFolder, Reason::KnownLocalFolder)
} else if extra_standard_library.contains(module_name) {
(ImportType::StandardLibrary, Reason::ExtraStandardLibrary)
} else if known_first_party.contains(module_base) {
(ImportType::FirstParty, Reason::KnownFirstParty)
} else if known_third_party.contains(module_base) {
Expand All @@ -59,8 +70,6 @@ pub fn categorize(
(ImportType::LocalFolder, Reason::KnownLocalFolder)
} else if extra_standard_library.contains(module_base) {
(ImportType::StandardLibrary, Reason::ExtraStandardLibrary)
} else if module_base == "__future__" {
(ImportType::Future, Reason::Future)
} else if KNOWN_STANDARD_LIBRARY
.get(&target_version.as_tuple())
.unwrap()
Expand All @@ -77,7 +86,7 @@ pub fn categorize(
};
debug!(
"Categorized '{}' as {:?} ({:?})",
module_base, import_type, reason
module_name, import_type, reason
);
import_type
}
Expand Down Expand Up @@ -117,7 +126,7 @@ pub fn categorize_imports<'a>(
// Categorize `StmtKind::Import`.
for (alias, comments) in block.import {
let import_type = categorize(
&alias.module_base(),
&alias.module_name(),
None,
src,
package,
Expand All @@ -136,7 +145,7 @@ pub fn categorize_imports<'a>(
// Categorize `StmtKind::ImportFrom` (without re-export).
for (import_from, aliases) in block.import_from {
let classification = categorize(
&import_from.module_base(),
&import_from.module_name(),
import_from.level,
src,
package,
Expand All @@ -155,7 +164,7 @@ pub fn categorize_imports<'a>(
// Categorize `StmtKind::ImportFrom` (with re-export).
for ((import_from, alias), comments) in block.import_from_as {
let classification = categorize(
&import_from.module_base(),
&import_from.module_name(),
import_from.level,
src,
package,
Expand All @@ -174,7 +183,7 @@ pub fn categorize_imports<'a>(
// Categorize `StmtKind::ImportFrom` (with star).
for (import_from, comments) in block.import_from_star {
let classification = categorize(
&import_from.module_base(),
&import_from.module_name(),
import_from.level,
src,
package,
Expand Down
51 changes: 47 additions & 4 deletions crates/ruff/src/rules/isort/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -376,10 +376,6 @@ mod tests {
#[test_case(Path::new("preserve_tabs_2.py"))]
#[test_case(Path::new("relative_imports_order.py"))]
#[test_case(Path::new("reorder_within_section.py"))]
#[test_case(Path::new("separate_first_party_imports.py"))]
#[test_case(Path::new("separate_future_imports.py"))]
#[test_case(Path::new("separate_local_folder_imports.py"))]
#[test_case(Path::new("separate_third_party_imports.py"))]
#[test_case(Path::new("skip.py"))]
#[test_case(Path::new("skip_file.py"))]
#[test_case(Path::new("sort_similar_imports.py"))]
Expand All @@ -400,6 +396,53 @@ mod tests {
Ok(())
}

#[test_case(Path::new("separate_first_party_imports.py"))]
#[test_case(Path::new("separate_future_imports.py"))]
#[test_case(Path::new("separate_local_folder_imports.py"))]
#[test_case(Path::new("separate_third_party_imports.py"))]
fn separate_modules(path: &Path) -> Result<()> {
let snapshot = format!("{}", path.to_string_lossy());
let diagnostics = test_path(
Path::new("isort").join(path).as_path(),
&Settings {
isort: super::settings::Settings {
known_first_party: BTreeSet::from(["foo.bar".to_string(), "baz".to_string()]),
known_third_party: BTreeSet::from([
"foo".to_string(),
"__future__".to_string(),
]),
..super::settings::Settings::default()
},
src: vec![test_resource_path("fixtures/isort")],
..Settings::for_rule(Rule::UnsortedImports)
},
)?;
assert_yaml_snapshot!(snapshot, diagnostics);
Ok(())
}

#[test_case(Path::new("separate_first_party_imports.py"))]
#[test_case(Path::new("separate_future_imports.py"))]
#[test_case(Path::new("separate_local_folder_imports.py"))]
#[test_case(Path::new("separate_third_party_imports.py"))]
fn separate_modules_first_party(path: &Path) -> Result<()> {
let snapshot = format!("{}_firty_party", path.to_string_lossy());
let diagnostics = test_path(
Path::new("isort").join(path).as_path(),
&Settings {
isort: super::settings::Settings {
known_first_party: BTreeSet::from(["foo".to_string()]),
known_third_party: BTreeSet::from(["foo.bar".to_string()]),
..super::settings::Settings::default()
},
src: vec![test_resource_path("fixtures/isort")],
..Settings::for_rule(Rule::UnsortedImports)
},
)?;
assert_yaml_snapshot!(snapshot, diagnostics);
Ok(())
}

// Test currently disabled as line endings are automatically converted to
// platform-appropriate ones in CI/CD #[test_case(Path::new("
// line_ending_crlf.py"))] #[test_case(Path::new("line_ending_lf.py"))]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,15 @@ expression: diagnostics
row: 1
column: 0
end_location:
row: 6
row: 13
column: 0
fix:
content: "import os\nimport sys\n\nimport numpy as np\n\nimport leading_prefix\nfrom leading_prefix import Class\n"
content: "import os\nimport sys\n\nimport foo\nimport foo.bar.baz\nimport numpy as np\nfrom foo import bar, baz\nfrom foo.bar.baz import something\n\nimport baz\nimport foo.bar\nimport leading_prefix\nfrom foo.bar import blah, blub\nfrom leading_prefix import Class\n"
location:
row: 1
column: 0
end_location:
row: 6
row: 13
column: 0
parent: ~

Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
---
source: crates/ruff/src/rules/isort/mod.rs
expression: diagnostics
---
- kind:
UnsortedImports: ~
location:
row: 1
column: 0
end_location:
row: 13
column: 0
fix:
content: "import os\nimport sys\n\nimport baz\nimport foo.bar\nimport numpy as np\nfrom foo.bar import blah, blub\n\nimport foo\nimport foo.bar.baz\nimport leading_prefix\nfrom foo import bar, baz\nfrom foo.bar.baz import something\nfrom leading_prefix import Class\n"
location:
row: 1
column: 0
end_location:
row: 13
column: 0
parent: ~

Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
---
source: crates/ruff/src/rules/isort/mod.rs
expression: diagnostics
---
- kind:
UnsortedImports: ~
location:
row: 1
column: 0
end_location:
row: 4
column: 0
fix:
content: "from __future__ import annotations\n\nimport os\nimport sys\n"
location:
row: 1
column: 0
end_location:
row: 4
column: 0
parent: ~

Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
---
source: crates/ruff/src/rules/isort/mod.rs
expression: diagnostics
---
- kind:
UnsortedImports: ~
location:
row: 1
column: 0
end_location:
row: 6
column: 0
fix:
content: "import os\nimport sys\n\nimport ruff\n\nimport leading_prefix\n\nfrom . import leading_prefix\n"
location:
row: 1
column: 0
end_location:
row: 6
column: 0
parent: ~

Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,15 @@ expression: diagnostics
row: 1
column: 0
end_location:
row: 5
row: 6
column: 0
fix:
content: "import os\nimport sys\n\nimport numpy as np\nimport pandas as pd\n"
content: "import os\nimport sys\n\nimport numpy as np\nimport pandas as pd\n\nimport foo.bar\n"
location:
row: 1
column: 0
end_location:
row: 5
row: 6
column: 0
parent: ~

Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
---
source: crates/ruff/src/rules/isort/mod.rs
expression: diagnostics
---
- kind:
UnsortedImports: ~
location:
row: 1
column: 0
end_location:
row: 6
column: 0
fix:
content: "import os\nimport sys\n\nimport foo.bar\nimport numpy as np\nimport pandas as pd\n"
location:
row: 1
column: 0
end_location:
row: 6
column: 0
parent: ~

0 comments on commit 594a8f5

Please sign in to comment.