Skip to content

Commit

Permalink
isort: Add support for the from-first setting (#8663)
Browse files Browse the repository at this point in the history
# Summary

This setting behaves similarly to the ``from_first`` setting in isort
upstream, and sorts "from X import Y" type imports before straight
imports.

Like the other PR I added, happy to refactor if this is better in
another form.

Fixes #8662 

# Test plan

I've added a unit test, and ran this on a large codebase that relies on
this setting in isort to verify it doesn't have unexpected side effects.
  • Loading branch information
jelmer authored Nov 21, 2023
1 parent 5ce6299 commit f1ed0f2
Show file tree
Hide file tree
Showing 7 changed files with 113 additions and 11 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
from __future__ import blah

from os import path

import os
60 changes: 54 additions & 6 deletions crates/ruff_linter/src/rules/isort/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,15 @@ fn format_import_block(
target_version: PythonVersion,
settings: &Settings,
) -> String {
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
enum LineInsertion {
/// A blank line should be inserted as soon as the next import is
/// of a different type (i.e., direct vs. `from`).
Necessary,
/// A blank line has already been inserted.
Inserted,
}

// Categorize by type (e.g., first-party vs. third-party).
let mut block_by_type = categorize_imports(
block,
Expand Down Expand Up @@ -182,31 +191,47 @@ fn format_import_block(
pending_lines_before = false;
}

let mut lines_inserted = false;
let mut has_direct_import = false;
let mut line_insertion = None;
let mut is_first_statement = true;
let lines_between_types = settings.lines_between_types;
for import in imports {
match import {
Import((alias, comments)) => {
// Add a blank lines between direct and from imports.
if settings.from_first
&& lines_between_types > 0
&& line_insertion == Some(LineInsertion::Necessary)
{
for _ in 0..lines_between_types {
output.push_str(&stylist.line_ending());
}

line_insertion = Some(LineInsertion::Inserted);
}

output.push_str(&format::format_import(
&alias,
&comments,
is_first_statement,
stylist,
));

has_direct_import = true;
if !settings.from_first {
line_insertion = Some(LineInsertion::Necessary);
}
}

ImportFrom((import_from, comments, trailing_comma, aliases)) => {
// Add a blank lines between direct and from imports
if lines_between_types > 0 && has_direct_import && !lines_inserted {
// Add a blank lines between direct and from imports.
if !settings.from_first
&& lines_between_types > 0
&& line_insertion == Some(LineInsertion::Necessary)
{
for _ in 0..lines_between_types {
output.push_str(&stylist.line_ending());
}

lines_inserted = true;
line_insertion = Some(LineInsertion::Inserted);
}

output.push_str(&format::format_import_from(
Expand All @@ -221,6 +246,10 @@ fn format_import_block(
settings.split_on_trailing_comma
&& matches!(trailing_comma, TrailingComma::Present),
));

if settings.from_first {
line_insertion = Some(LineInsertion::Necessary);
}
}
}
is_first_statement = false;
Expand Down Expand Up @@ -819,6 +848,25 @@ mod tests {
Ok(())
}

#[test_case(Path::new("from_first.py"))]
fn from_first(path: &Path) -> Result<()> {
let snapshot = format!("from_first_{}", path.to_string_lossy());
let diagnostics = test_path(
Path::new("isort").join(path).as_path(),
&LinterSettings {
isort: super::settings::Settings {
from_first: true,
lines_between_types: 1,
..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("relative_imports_order.py"))]
fn closest_to_furthest(path: &Path) -> Result<()> {
let snapshot = format!("closest_to_furthest_{}", path.to_string_lossy());
Expand Down
18 changes: 13 additions & 5 deletions crates/ruff_linter/src/rules/isort/order.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,19 @@ pub(crate) fn order_imports<'a>(
settings,
)
});
ordered_straight_imports
.into_iter()
.map(Import)
.chain(ordered_from_imports.into_iter().map(ImportFrom))
.collect()
if settings.from_first {
ordered_from_imports
.into_iter()
.map(ImportFrom)
.chain(ordered_straight_imports.into_iter().map(Import))
.collect()
} else {
ordered_straight_imports
.into_iter()
.map(Import)
.chain(ordered_from_imports.into_iter().map(ImportFrom))
.collect()
}
};

ordered_imports
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/isort/settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ pub struct Settings {
pub forced_separate: Vec<String>,
pub section_order: Vec<ImportSection>,
pub no_sections: bool,
pub from_first: bool,
}

impl Default for Settings {
Expand Down Expand Up @@ -84,6 +85,7 @@ impl Default for Settings {
forced_separate: Vec::new(),
section_order: ImportType::iter().map(ImportSection::Known).collect(),
no_sections: false,
from_first: false,
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
source: crates/ruff_linter/src/rules/isort/mod.rs
---

28 changes: 28 additions & 0 deletions crates/ruff_workspace/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2021,6 +2021,32 @@ pub struct IsortOptions {
)]
pub detect_same_package: Option<bool>,

/// Whether to place `import from` imports before straight imports when sorting.
///
/// For example, by default, imports will be sorted such that straight imports appear
/// before `import from` imports, as in:
/// ```python
/// import os
/// import sys
/// from typing import List
/// ```
///
/// Setting `from-first = true` will instead sort such that `import from` imports appear
/// before straight imports, as in:
/// ```python
/// from typing import List
/// import os
/// import sys
/// ```
#[option(
default = r#"false"#,
value_type = "bool",
example = r#"
from-first = true
"#
)]
pub from_first: Option<bool>,

// Tables are required to go last.
/// A list of mappings from section names to modules.
/// By default custom sections are output last, but this can be overridden with `section-order`.
Expand Down Expand Up @@ -2098,6 +2124,7 @@ impl IsortOptions {
.map_err(isort::settings::SettingsError::InvalidExtraStandardLibrary)?
.unwrap_or_default();
let no_lines_before = self.no_lines_before.unwrap_or_default();
let from_first = self.from_first.unwrap_or_default();
let sections = self.sections.unwrap_or_default();

// Verify that `sections` doesn't contain any built-in sections.
Expand Down Expand Up @@ -2206,6 +2233,7 @@ impl IsortOptions {
forced_separate: Vec::from_iter(self.forced_separate.unwrap_or_default()),
section_order,
no_sections,
from_first,
})
}
}
Expand Down
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 f1ed0f2

Please sign in to comment.