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

fix(isort): comply with isort when sorting sub-package modules #3064

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
@@ -0,0 +1,8 @@
import sys
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 @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

What does isort do if you have:

known-first-party = ["foo"]
known-third-party = ["foo.bar"]

And then you import foo.bar.baz. Does that get classified as first- or third-party?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[settings]
known_first_party=foo
known_third_party=foo.bar

results in:

import sys

import foo.bar
import foo.bar.baz
from foo.bar import blah, blub
from foo.bar.baz import something

import foo
from foo import bar, baz

So my MR currently has this diff:

ruff --select I --fix --diff foo.py 
--- foo.py
+++ foo.py
@@ -1,9 +1,9 @@
 import sys
 
 import foo.bar
-import foo.bar.baz
 from foo.bar import blah, blub
-from foo.bar.baz import something
 
 import foo
+import foo.bar.baz
 from foo import bar, baz
+from foo.bar.baz import something

Would fix 1 error.

} 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
41 changes: 41 additions & 0 deletions crates/ruff/src/rules/isort/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,47 @@ mod tests {
Ok(())
}

#[test_case(Path::new("separate_subpackage_first_and_third_party_imports.py"))]
fn separate_modules(path: &Path) -> Result<()> {
let snapshot = format!("1_{}", 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_subpackage_first_and_third_party_imports.py"))]
fn separate_modules_first_party(path: &Path) -> Result<()> {
let snapshot = format!("2_{}", 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
@@ -0,0 +1,22 @@
---
source: crates/ruff/src/rules/isort/mod.rs
expression: diagnostics
---
- kind:
UnsortedImports: ~
location:
row: 1
column: 0
end_location:
row: 9
column: 0
fix:
content: "import sys\n\nimport foo\nfrom foo import bar, baz\n\nimport baz\nimport foo.bar\nimport foo.bar.baz\nfrom foo.bar import blah, blub\nfrom foo.bar.baz import something\n"
location:
row: 1
column: 0
end_location:
row: 9
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: 9
column: 0
fix:
content: "import sys\n\nimport baz\nimport foo.bar\nimport foo.bar.baz\nfrom foo.bar import blah, blub\nfrom foo.bar.baz import something\n\nimport foo\nfrom foo import bar, baz\n"
location:
row: 1
column: 0
end_location:
row: 9
column: 0
parent: ~