mirror of https://github.com/astral-sh/ruff
Make isort's `detect-same-package` behavior configurable (#6833)
## Summary Our first-party import detection uses a heuristic that doesn't exist in isort: if an import appears to be from within the same package as the containing file, we mark it as first-party. For example, if you have a directory `./foo/__init__.py`, and you import `from foo import bar` in `./foo/baz.py`, we'll mark that as first-party. (See: https://github.com/astral-sh/ruff/pull/1266.) This is often unnecessary, and arguably should be removed (though it does have some important use-cases that are otherwise unserved -- I believe Dagster uses it to ensure that all packages mark imports from within the same package as first-party, but not imports _across_ different first-party packages)... but it does exist, and it does help in cases in which the `src` field is not properly configured. This PR adds an option to turn off this behavior: ```toml [tool.ruff.isort] detect-same-package = false ``` This is being introduced to help codebases migrating over from isort that may want more consistent behavior with their current sorting. ## Test Plan `cargo test`
This commit is contained in:
parent
3bd199cdf6
commit
281ce56dc1
0
crates/ruff/resources/test/fixtures/isort/detect_same_package/foo/__init__.py
vendored
Normal file
0
crates/ruff/resources/test/fixtures/isort/detect_same_package/foo/__init__.py
vendored
Normal file
|
|
@ -0,0 +1,3 @@
|
||||||
|
import os
|
||||||
|
import pandas
|
||||||
|
import foo.baz
|
||||||
|
|
@ -0,0 +1,2 @@
|
||||||
|
[tool.ruff]
|
||||||
|
line-length = 88
|
||||||
|
|
@ -283,6 +283,7 @@ pub(crate) fn typing_only_runtime_import(
|
||||||
None,
|
None,
|
||||||
&checker.settings.src,
|
&checker.settings.src,
|
||||||
checker.package(),
|
checker.package(),
|
||||||
|
checker.settings.isort.detect_same_package,
|
||||||
&checker.settings.isort.known_modules,
|
&checker.settings.isort.known_modules,
|
||||||
checker.settings.target_version,
|
checker.settings.target_version,
|
||||||
) {
|
) {
|
||||||
|
|
|
||||||
|
|
@ -69,6 +69,7 @@ pub(crate) fn categorize<'a>(
|
||||||
level: Option<u32>,
|
level: Option<u32>,
|
||||||
src: &[PathBuf],
|
src: &[PathBuf],
|
||||||
package: Option<&Path>,
|
package: Option<&Path>,
|
||||||
|
detect_same_package: bool,
|
||||||
known_modules: &'a KnownModules,
|
known_modules: &'a KnownModules,
|
||||||
target_version: PythonVersion,
|
target_version: PythonVersion,
|
||||||
) -> &'a ImportSection {
|
) -> &'a ImportSection {
|
||||||
|
|
@ -88,7 +89,7 @@ pub(crate) fn categorize<'a>(
|
||||||
&ImportSection::Known(ImportType::StandardLibrary),
|
&ImportSection::Known(ImportType::StandardLibrary),
|
||||||
Reason::KnownStandardLibrary,
|
Reason::KnownStandardLibrary,
|
||||||
)
|
)
|
||||||
} else if same_package(package, module_base) {
|
} else if detect_same_package && same_package(package, module_base) {
|
||||||
(
|
(
|
||||||
&ImportSection::Known(ImportType::FirstParty),
|
&ImportSection::Known(ImportType::FirstParty),
|
||||||
Reason::SamePackage,
|
Reason::SamePackage,
|
||||||
|
|
@ -137,6 +138,7 @@ pub(crate) fn categorize_imports<'a>(
|
||||||
block: ImportBlock<'a>,
|
block: ImportBlock<'a>,
|
||||||
src: &[PathBuf],
|
src: &[PathBuf],
|
||||||
package: Option<&Path>,
|
package: Option<&Path>,
|
||||||
|
detect_same_package: bool,
|
||||||
known_modules: &'a KnownModules,
|
known_modules: &'a KnownModules,
|
||||||
target_version: PythonVersion,
|
target_version: PythonVersion,
|
||||||
) -> BTreeMap<&'a ImportSection, ImportBlock<'a>> {
|
) -> BTreeMap<&'a ImportSection, ImportBlock<'a>> {
|
||||||
|
|
@ -148,6 +150,7 @@ pub(crate) fn categorize_imports<'a>(
|
||||||
None,
|
None,
|
||||||
src,
|
src,
|
||||||
package,
|
package,
|
||||||
|
detect_same_package,
|
||||||
known_modules,
|
known_modules,
|
||||||
target_version,
|
target_version,
|
||||||
);
|
);
|
||||||
|
|
@ -164,6 +167,7 @@ pub(crate) fn categorize_imports<'a>(
|
||||||
import_from.level,
|
import_from.level,
|
||||||
src,
|
src,
|
||||||
package,
|
package,
|
||||||
|
detect_same_package,
|
||||||
known_modules,
|
known_modules,
|
||||||
target_version,
|
target_version,
|
||||||
);
|
);
|
||||||
|
|
@ -180,6 +184,7 @@ pub(crate) fn categorize_imports<'a>(
|
||||||
import_from.level,
|
import_from.level,
|
||||||
src,
|
src,
|
||||||
package,
|
package,
|
||||||
|
detect_same_package,
|
||||||
known_modules,
|
known_modules,
|
||||||
target_version,
|
target_version,
|
||||||
);
|
);
|
||||||
|
|
@ -196,6 +201,7 @@ pub(crate) fn categorize_imports<'a>(
|
||||||
import_from.level,
|
import_from.level,
|
||||||
src,
|
src,
|
||||||
package,
|
package,
|
||||||
|
detect_same_package,
|
||||||
known_modules,
|
known_modules,
|
||||||
target_version,
|
target_version,
|
||||||
);
|
);
|
||||||
|
|
|
||||||
|
|
@ -82,6 +82,7 @@ pub(crate) fn format_imports(
|
||||||
force_to_top: &BTreeSet<String>,
|
force_to_top: &BTreeSet<String>,
|
||||||
known_modules: &KnownModules,
|
known_modules: &KnownModules,
|
||||||
order_by_type: bool,
|
order_by_type: bool,
|
||||||
|
detect_same_package: bool,
|
||||||
relative_imports_order: RelativeImportsOrder,
|
relative_imports_order: RelativeImportsOrder,
|
||||||
single_line_exclusions: &BTreeSet<String>,
|
single_line_exclusions: &BTreeSet<String>,
|
||||||
split_on_trailing_comma: bool,
|
split_on_trailing_comma: bool,
|
||||||
|
|
@ -129,6 +130,7 @@ pub(crate) fn format_imports(
|
||||||
force_to_top,
|
force_to_top,
|
||||||
known_modules,
|
known_modules,
|
||||||
order_by_type,
|
order_by_type,
|
||||||
|
detect_same_package,
|
||||||
relative_imports_order,
|
relative_imports_order,
|
||||||
split_on_trailing_comma,
|
split_on_trailing_comma,
|
||||||
classes,
|
classes,
|
||||||
|
|
@ -187,6 +189,7 @@ fn format_import_block(
|
||||||
force_to_top: &BTreeSet<String>,
|
force_to_top: &BTreeSet<String>,
|
||||||
known_modules: &KnownModules,
|
known_modules: &KnownModules,
|
||||||
order_by_type: bool,
|
order_by_type: bool,
|
||||||
|
detect_same_package: bool,
|
||||||
relative_imports_order: RelativeImportsOrder,
|
relative_imports_order: RelativeImportsOrder,
|
||||||
split_on_trailing_comma: bool,
|
split_on_trailing_comma: bool,
|
||||||
classes: &BTreeSet<String>,
|
classes: &BTreeSet<String>,
|
||||||
|
|
@ -198,7 +201,14 @@ fn format_import_block(
|
||||||
section_order: &[ImportSection],
|
section_order: &[ImportSection],
|
||||||
) -> String {
|
) -> String {
|
||||||
// Categorize by type (e.g., first-party vs. third-party).
|
// Categorize by type (e.g., first-party vs. third-party).
|
||||||
let mut block_by_type = categorize_imports(block, src, package, known_modules, target_version);
|
let mut block_by_type = categorize_imports(
|
||||||
|
block,
|
||||||
|
src,
|
||||||
|
package,
|
||||||
|
detect_same_package,
|
||||||
|
known_modules,
|
||||||
|
target_version,
|
||||||
|
);
|
||||||
|
|
||||||
let mut output = String::new();
|
let mut output = String::new();
|
||||||
|
|
||||||
|
|
@ -1084,4 +1094,38 @@ mod tests {
|
||||||
assert_messages!(snapshot, diagnostics);
|
assert_messages!(snapshot, diagnostics);
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn detect_same_package() -> Result<()> {
|
||||||
|
let diagnostics = test_path(
|
||||||
|
Path::new("isort/detect_same_package/foo/bar.py"),
|
||||||
|
&Settings {
|
||||||
|
src: vec![],
|
||||||
|
isort: super::settings::Settings {
|
||||||
|
detect_same_package: true,
|
||||||
|
..super::settings::Settings::default()
|
||||||
|
},
|
||||||
|
..Settings::for_rule(Rule::UnsortedImports)
|
||||||
|
},
|
||||||
|
)?;
|
||||||
|
assert_messages!(diagnostics);
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn no_detect_same_package() -> Result<()> {
|
||||||
|
let diagnostics = test_path(
|
||||||
|
Path::new("isort/detect_same_package/foo/bar.py"),
|
||||||
|
&Settings {
|
||||||
|
src: vec![],
|
||||||
|
isort: super::settings::Settings {
|
||||||
|
detect_same_package: false,
|
||||||
|
..super::settings::Settings::default()
|
||||||
|
},
|
||||||
|
..Settings::for_rule(Rule::UnsortedImports)
|
||||||
|
},
|
||||||
|
)?;
|
||||||
|
assert_messages!(diagnostics);
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -134,6 +134,7 @@ pub(crate) fn organize_imports(
|
||||||
&settings.isort.force_to_top,
|
&settings.isort.force_to_top,
|
||||||
&settings.isort.known_modules,
|
&settings.isort.known_modules,
|
||||||
settings.isort.order_by_type,
|
settings.isort.order_by_type,
|
||||||
|
settings.isort.detect_same_package,
|
||||||
settings.isort.relative_imports_order,
|
settings.isort.relative_imports_order,
|
||||||
&settings.isort.single_line_exclusions,
|
&settings.isort.single_line_exclusions,
|
||||||
settings.isort.split_on_trailing_comma,
|
settings.isort.split_on_trailing_comma,
|
||||||
|
|
|
||||||
|
|
@ -305,6 +305,21 @@ pub struct Options {
|
||||||
)]
|
)]
|
||||||
/// Override in which order the sections should be output. Can be used to move custom sections.
|
/// Override in which order the sections should be output. Can be used to move custom sections.
|
||||||
pub section_order: Option<Vec<ImportSection>>,
|
pub section_order: Option<Vec<ImportSection>>,
|
||||||
|
#[option(
|
||||||
|
default = r#"true"#,
|
||||||
|
value_type = "bool",
|
||||||
|
example = r#"
|
||||||
|
detect-same-package = false
|
||||||
|
"#
|
||||||
|
)]
|
||||||
|
/// Whether to automatically mark imports from within the same package as first-party.
|
||||||
|
/// For example, when `detect-same-package = true`, then when analyzing files within the
|
||||||
|
/// `foo` package, any imports from within the `foo` package will be considered first-party.
|
||||||
|
///
|
||||||
|
/// This heuristic is often unnecessary when `src` is configured to detect all first-party
|
||||||
|
/// sources; however, if `src` is _not_ configured, this heuristic can be useful to detect
|
||||||
|
/// first-party imports from _within_ (but not _across_) first-party packages.
|
||||||
|
pub detect_same_package: Option<bool>,
|
||||||
// Tables are required to go last.
|
// Tables are required to go last.
|
||||||
#[option(
|
#[option(
|
||||||
default = "{}",
|
default = "{}",
|
||||||
|
|
@ -331,6 +346,7 @@ pub struct Settings {
|
||||||
pub force_wrap_aliases: bool,
|
pub force_wrap_aliases: bool,
|
||||||
pub force_to_top: BTreeSet<String>,
|
pub force_to_top: BTreeSet<String>,
|
||||||
pub known_modules: KnownModules,
|
pub known_modules: KnownModules,
|
||||||
|
pub detect_same_package: bool,
|
||||||
pub order_by_type: bool,
|
pub order_by_type: bool,
|
||||||
pub relative_imports_order: RelativeImportsOrder,
|
pub relative_imports_order: RelativeImportsOrder,
|
||||||
pub single_line_exclusions: BTreeSet<String>,
|
pub single_line_exclusions: BTreeSet<String>,
|
||||||
|
|
@ -352,6 +368,7 @@ impl Default for Settings {
|
||||||
combine_as_imports: false,
|
combine_as_imports: false,
|
||||||
force_single_line: false,
|
force_single_line: false,
|
||||||
force_sort_within_sections: false,
|
force_sort_within_sections: false,
|
||||||
|
detect_same_package: true,
|
||||||
case_sensitive: false,
|
case_sensitive: false,
|
||||||
force_wrap_aliases: false,
|
force_wrap_aliases: false,
|
||||||
force_to_top: BTreeSet::new(),
|
force_to_top: BTreeSet::new(),
|
||||||
|
|
@ -509,6 +526,7 @@ impl TryFrom<Options> for Settings {
|
||||||
force_sort_within_sections: options.force_sort_within_sections.unwrap_or(false),
|
force_sort_within_sections: options.force_sort_within_sections.unwrap_or(false),
|
||||||
case_sensitive: options.case_sensitive.unwrap_or(false),
|
case_sensitive: options.case_sensitive.unwrap_or(false),
|
||||||
force_wrap_aliases: options.force_wrap_aliases.unwrap_or(false),
|
force_wrap_aliases: options.force_wrap_aliases.unwrap_or(false),
|
||||||
|
detect_same_package: options.detect_same_package.unwrap_or(true),
|
||||||
force_to_top: BTreeSet::from_iter(options.force_to_top.unwrap_or_default()),
|
force_to_top: BTreeSet::from_iter(options.force_to_top.unwrap_or_default()),
|
||||||
known_modules: KnownModules::new(
|
known_modules: KnownModules::new(
|
||||||
known_first_party,
|
known_first_party,
|
||||||
|
|
@ -595,6 +613,7 @@ impl From<Settings> for Options {
|
||||||
force_sort_within_sections: Some(settings.force_sort_within_sections),
|
force_sort_within_sections: Some(settings.force_sort_within_sections),
|
||||||
case_sensitive: Some(settings.case_sensitive),
|
case_sensitive: Some(settings.case_sensitive),
|
||||||
force_wrap_aliases: Some(settings.force_wrap_aliases),
|
force_wrap_aliases: Some(settings.force_wrap_aliases),
|
||||||
|
detect_same_package: Some(settings.detect_same_package),
|
||||||
force_to_top: Some(settings.force_to_top.into_iter().collect()),
|
force_to_top: Some(settings.force_to_top.into_iter().collect()),
|
||||||
known_first_party: Some(
|
known_first_party: Some(
|
||||||
settings
|
settings
|
||||||
|
|
|
||||||
|
|
@ -0,0 +1,19 @@
|
||||||
|
---
|
||||||
|
source: crates/ruff/src/rules/isort/mod.rs
|
||||||
|
---
|
||||||
|
bar.py:1:1: I001 [*] Import block is un-sorted or un-formatted
|
||||||
|
|
|
||||||
|
1 | / import os
|
||||||
|
2 | | import pandas
|
||||||
|
3 | | import foo.baz
|
||||||
|
|
|
||||||
|
= help: Organize imports
|
||||||
|
|
||||||
|
ℹ Fix
|
||||||
|
1 1 | import os
|
||||||
|
2 |+
|
||||||
|
2 3 | import pandas
|
||||||
|
4 |+
|
||||||
|
3 5 | import foo.baz
|
||||||
|
|
||||||
|
|
||||||
|
|
@ -0,0 +1,19 @@
|
||||||
|
---
|
||||||
|
source: crates/ruff/src/rules/isort/mod.rs
|
||||||
|
---
|
||||||
|
bar.py:1:1: I001 [*] Import block is un-sorted or un-formatted
|
||||||
|
|
|
||||||
|
1 | / import os
|
||||||
|
2 | | import pandas
|
||||||
|
3 | | import foo.baz
|
||||||
|
|
|
||||||
|
= help: Organize imports
|
||||||
|
|
||||||
|
ℹ Fix
|
||||||
|
1 1 | import os
|
||||||
|
2 |-import pandas
|
||||||
|
2 |+
|
||||||
|
3 3 | import foo.baz
|
||||||
|
4 |+import pandas
|
||||||
|
|
||||||
|
|
||||||
|
|
@ -1189,6 +1189,13 @@
|
||||||
"type": "string"
|
"type": "string"
|
||||||
}
|
}
|
||||||
},
|
},
|
||||||
|
"detect-same-package": {
|
||||||
|
"description": "Whether to automatically mark imports from within the same package as first-party. For example, when `detect-same-package = true`, then when analyzing files within the `foo` package, any imports from within the `foo` package will be considered first-party.\n\nThis heuristic is often unnecessary when `src` is configured to detect all first-party sources; however, if `src` is _not_ configured, this heuristic can be useful to detect first-party imports from _within_ (but not _across_) first-party packages.",
|
||||||
|
"type": [
|
||||||
|
"boolean",
|
||||||
|
"null"
|
||||||
|
]
|
||||||
|
},
|
||||||
"extra-standard-library": {
|
"extra-standard-library": {
|
||||||
"description": "A list of modules to consider standard-library, in addition to those known to Ruff in advance.\n\nSupports glob patterns. For more information on the glob syntax, refer to the [`globset` documentation](https://docs.rs/globset/latest/globset/#syntax).",
|
"description": "A list of modules to consider standard-library, in addition to those known to Ruff in advance.\n\nSupports glob patterns. For more information on the glob syntax, refer to the [`globset` documentation](https://docs.rs/globset/latest/globset/#syntax).",
|
||||||
"type": [
|
"type": [
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue