Skip to content

Commit

Permalink
Make isort's detect-same-package behavior configurable (#6833)
Browse files Browse the repository at this point in the history
## 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:
#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`
  • Loading branch information
charliermarsh committed Aug 24, 2023
1 parent 3bd199c commit 281ce56
Show file tree
Hide file tree
Showing 11 changed files with 123 additions and 2 deletions.
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import os
import pandas
import foo.baz
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[tool.ruff]
line-length = 88
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,7 @@ pub(crate) fn typing_only_runtime_import(
None,
&checker.settings.src,
checker.package(),
checker.settings.isort.detect_same_package,
&checker.settings.isort.known_modules,
checker.settings.target_version,
) {
Expand Down
8 changes: 7 additions & 1 deletion crates/ruff/src/rules/isort/categorize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ pub(crate) fn categorize<'a>(
level: Option<u32>,
src: &[PathBuf],
package: Option<&Path>,
detect_same_package: bool,
known_modules: &'a KnownModules,
target_version: PythonVersion,
) -> &'a ImportSection {
Expand All @@ -88,7 +89,7 @@ pub(crate) fn categorize<'a>(
&ImportSection::Known(ImportType::StandardLibrary),
Reason::KnownStandardLibrary,
)
} else if same_package(package, module_base) {
} else if detect_same_package && same_package(package, module_base) {
(
&ImportSection::Known(ImportType::FirstParty),
Reason::SamePackage,
Expand Down Expand Up @@ -137,6 +138,7 @@ pub(crate) fn categorize_imports<'a>(
block: ImportBlock<'a>,
src: &[PathBuf],
package: Option<&Path>,
detect_same_package: bool,
known_modules: &'a KnownModules,
target_version: PythonVersion,
) -> BTreeMap<&'a ImportSection, ImportBlock<'a>> {
Expand All @@ -148,6 +150,7 @@ pub(crate) fn categorize_imports<'a>(
None,
src,
package,
detect_same_package,
known_modules,
target_version,
);
Expand All @@ -164,6 +167,7 @@ pub(crate) fn categorize_imports<'a>(
import_from.level,
src,
package,
detect_same_package,
known_modules,
target_version,
);
Expand All @@ -180,6 +184,7 @@ pub(crate) fn categorize_imports<'a>(
import_from.level,
src,
package,
detect_same_package,
known_modules,
target_version,
);
Expand All @@ -196,6 +201,7 @@ pub(crate) fn categorize_imports<'a>(
import_from.level,
src,
package,
detect_same_package,
known_modules,
target_version,
);
Expand Down
46 changes: 45 additions & 1 deletion crates/ruff/src/rules/isort/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ pub(crate) fn format_imports(
force_to_top: &BTreeSet<String>,
known_modules: &KnownModules,
order_by_type: bool,
detect_same_package: bool,
relative_imports_order: RelativeImportsOrder,
single_line_exclusions: &BTreeSet<String>,
split_on_trailing_comma: bool,
Expand Down Expand Up @@ -129,6 +130,7 @@ pub(crate) fn format_imports(
force_to_top,
known_modules,
order_by_type,
detect_same_package,
relative_imports_order,
split_on_trailing_comma,
classes,
Expand Down Expand Up @@ -187,6 +189,7 @@ fn format_import_block(
force_to_top: &BTreeSet<String>,
known_modules: &KnownModules,
order_by_type: bool,
detect_same_package: bool,
relative_imports_order: RelativeImportsOrder,
split_on_trailing_comma: bool,
classes: &BTreeSet<String>,
Expand All @@ -198,7 +201,14 @@ fn format_import_block(
section_order: &[ImportSection],
) -> String {
// 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();

Expand Down Expand Up @@ -1084,4 +1094,38 @@ mod tests {
assert_messages!(snapshot, diagnostics);
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(())
}
}
1 change: 1 addition & 0 deletions crates/ruff/src/rules/isort/rules/organize_imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ pub(crate) fn organize_imports(
&settings.isort.force_to_top,
&settings.isort.known_modules,
settings.isort.order_by_type,
settings.isort.detect_same_package,
settings.isort.relative_imports_order,
&settings.isort.single_line_exclusions,
settings.isort.split_on_trailing_comma,
Expand Down
19 changes: 19 additions & 0 deletions crates/ruff/src/rules/isort/settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,21 @@ pub struct Options {
)]
/// Override in which order the sections should be output. Can be used to move custom sections.
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.
#[option(
default = "{}",
Expand All @@ -331,6 +346,7 @@ pub struct Settings {
pub force_wrap_aliases: bool,
pub force_to_top: BTreeSet<String>,
pub known_modules: KnownModules,
pub detect_same_package: bool,
pub order_by_type: bool,
pub relative_imports_order: RelativeImportsOrder,
pub single_line_exclusions: BTreeSet<String>,
Expand All @@ -352,6 +368,7 @@ impl Default for Settings {
combine_as_imports: false,
force_single_line: false,
force_sort_within_sections: false,
detect_same_package: true,
case_sensitive: false,
force_wrap_aliases: false,
force_to_top: BTreeSet::new(),
Expand Down Expand Up @@ -509,6 +526,7 @@ impl TryFrom<Options> for Settings {
force_sort_within_sections: options.force_sort_within_sections.unwrap_or(false),
case_sensitive: options.case_sensitive.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()),
known_modules: KnownModules::new(
known_first_party,
Expand Down Expand Up @@ -595,6 +613,7 @@ impl From<Settings> for Options {
force_sort_within_sections: Some(settings.force_sort_within_sections),
case_sensitive: Some(settings.case_sensitive),
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()),
known_first_party: Some(
settings
Expand Down
Original file line number Diff line number Diff line change
@@ -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


Original file line number Diff line number Diff line change
@@ -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


7 changes: 7 additions & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 281ce56

Please sign in to comment.