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.

Sign up for GitHub

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

Jump to bottom

Avoid infinite-propagation of inline comments when force-splitting imports #4074

Merged
merged 2 commits into from
Apr 24, 2023
Merged

Avoid infinite-propagation of inline comments when force-splitting imports #4074

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fed4bf0
Fix force-signle line
charliermarsh Apr 24, 2023
0bfb899
Reverts
charliermarsh Apr 24, 2023
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
4 changes: 4 additions & 0 deletions crates/ruff/resources/test/fixtures/isort/propagate_inline_comments.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
from mypackage.subpackage import ( # long comment that seems to be a problem
a_long_variable_name_that_causes_problems,
items,
)
86 changes: 26 additions & 60 deletions crates/ruff/src/rules/isort/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@
use std::collections::BTreeSet;
use std::path::{Path, PathBuf};

use itertools::Either::{Left, Right};

use annotate::annotate_imports;
use categorize::categorize_imports;
pub use categorize::{categorize, ImportSection, ImportType};
Expand All @@ -16,7 +14,7 @@ use settings::RelativeImportsOrder;
use sorting::cmp_either_import;
use track::{Block, Trailer};
use types::EitherImport::{Import, ImportFrom};
use types::{AliasData, CommentSet, EitherImport, OrderedImportBlock, TrailingComma};
use types::{AliasData, EitherImport, TrailingComma};

use crate::rules::isort::categorize::KnownModules;
use crate::rules::isort::types::ImportBlock;
Expand Down Expand Up @@ -61,53 +59,6 @@ pub enum AnnotatedImport<'a> {
},
}

fn force_single_line_imports<'a>(
block: OrderedImportBlock<'a>,
single_line_exclusions: &BTreeSet<String>,
) -> OrderedImportBlock<'a> {
OrderedImportBlock {
import: block.import,
import_from: block
.import_from
.into_iter()
.flat_map(|(from_data, comment_set, trailing_comma, alias_data)| {
if from_data
.module
.map_or(false, |module| single_line_exclusions.contains(module))
{
Left(std::iter::once((
from_data,
comment_set,
trailing_comma,
alias_data,
)))
} else {
Right(
alias_data
.into_iter()
.enumerate()
.map(move |(index, alias_data)| {
(
from_data.clone(),
if index == 0 {
comment_set.clone()
} else {
CommentSet {
atop: vec![],
inline: comment_set.inline.clone(),
}
},
TrailingComma::Absent,
vec![alias_data],
)
}),
)
}
})
.collect(),
}
}

#[allow(clippy::too_many_arguments, clippy::fn_params_excessive_bools)]
pub fn format_imports(
block: &Block,
Expand Down Expand Up @@ -141,7 +92,12 @@ pub fn format_imports(
let block = annotate_imports(&block.imports, comments, locator, split_on_trailing_comma);

// Normalize imports (i.e., deduplicate, aggregate `from` imports).
let block = normalize_imports(block, combine_as_imports, force_single_line);
let block = normalize_imports(
block,
combine_as_imports,
force_single_line,
single_line_exclusions,
);

// Categorize imports.
let mut output = String::new();
Expand All @@ -153,14 +109,12 @@ pub fn format_imports(
stylist,
src,
package,
force_single_line,
force_sort_within_sections,
force_wrap_aliases,
force_to_top,
known_modules,
order_by_type,
relative_imports_order,
single_line_exclusions,
split_on_trailing_comma,
classes,
constants,
Expand Down Expand Up @@ -211,14 +165,12 @@ fn format_import_block(
stylist: &Stylist,
src: &[PathBuf],
package: Option<&Path>,
force_single_line: bool,
force_sort_within_sections: bool,
force_wrap_aliases: bool,
force_to_top: &BTreeSet<String>,
known_modules: &KnownModules,
order_by_type: bool,
relative_imports_order: RelativeImportsOrder,
single_line_exclusions: &BTreeSet<String>,
split_on_trailing_comma: bool,
classes: &BTreeSet<String>,
constants: &BTreeSet<String>,
Expand Down Expand Up @@ -246,7 +198,7 @@ fn format_import_block(
continue;
};

let mut imports = order_imports(
let imports = order_imports(
import_block,
order_by_type,
relative_imports_order,
Expand All @@ -256,10 +208,6 @@ fn format_import_block(
force_to_top,
);

if force_single_line {
imports = force_single_line_imports(imports, single_line_exclusions);
}

let imports = {
let mut imports = imports
.import
Expand Down Expand Up @@ -594,6 +542,24 @@ mod tests {
Ok(())
}

#[test_case(Path::new("propagate_inline_comments.py"))]
fn propagate_inline_comments(path: &Path) -> Result<()> {
let snapshot = format!("propagate_inline_comments_{}", path.to_string_lossy());
let diagnostics = test_path(
Path::new("isort").join(path).as_path(),
&Settings {
isort: super::settings::Settings {
force_single_line: true,
..super::settings::Settings::default()
},
src: vec![test_resource_path("fixtures/isort")],
..Settings::for_rule(Rule::UnsortedImports)
},
)?;
assert_messages!(snapshot, diagnostics);
Ok(())
}

#[test_case(Path::new("order_by_type.py"))]
fn order_by_type(path: &Path) -> Result<()> {
let snapshot = format!("order_by_type_false_{}", path.to_string_lossy());
Expand Down
85 changes: 60 additions & 25 deletions crates/ruff/src/rules/isort/normalize.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
use crate::rules::isort::types::TrailingComma;
use std::collections::BTreeSet;

use super::types::{AliasData, ImportBlock, ImportFromData};
use super::AnnotatedImport;

pub fn normalize_imports(
imports: Vec<AnnotatedImport>,
pub fn normalize_imports<'a>(
imports: Vec<AnnotatedImport<'a>>,
combine_as_imports: bool,
force_single_line: bool,
) -> ImportBlock {
single_line_exclusions: &'a BTreeSet<String>,
) -> ImportBlock<'a> {
let mut block = ImportBlock::default();
for import in imports {
match import {
Expand Down Expand Up @@ -52,20 +54,15 @@ pub fn normalize_imports(
inline,
trailing_comma,
} => {
// Whether to track each member of the import as a separate entry.
let isolate_aliases = force_single_line
&& module.map_or(true, |module| !single_line_exclusions.contains(module));

// Insert comments on the statement itself.
if let Some(alias) = names.first() {
let import_from = if alias.name == "*" {
block
.import_from_star
.entry(ImportFromData { module, level })
.or_default()
} else if alias.asname.is_none() || combine_as_imports || force_single_line {
block
.import_from
.entry(ImportFromData { module, level })
.or_default()
} else {
block
if isolate_aliases {
let mut first = true;
for alias in &names {
let import_from = block
.import_from_as
.entry((
ImportFromData { module, level },
Expand All @@ -74,26 +71,64 @@ pub fn normalize_imports(
asname: alias.asname,
},
))
.or_default()
};
.or_default();

for comment in atop {
import_from.comments.atop.push(comment.value);
// Associate the comments above the import statement with the first alias
// (best effort).
if std::mem::take(&mut first) {
for comment in &atop {
import_from.comments.atop.push(comment.value.clone());
}
}

// Replicate the inline comments onto every member.
for comment in &inline {
import_from.comments.inline.push(comment.value.clone());
}
}
} else {
if let Some(alias) = names.first() {
let import_from = if alias.name == "*" {
block
.import_from_star
.entry(ImportFromData { module, level })
.or_default()
} else if alias.asname.is_none() || combine_as_imports {
block
.import_from
.entry(ImportFromData { module, level })
.or_default()
} else {
block
.import_from_as
.entry((
ImportFromData { module, level },
AliasData {
name: alias.name,
asname: alias.asname,
},
))
.or_default()
};

for comment in inline {
import_from.comments.inline.push(comment.value);
for comment in atop {
import_from.comments.atop.push(comment.value);
}

for comment in inline {
import_from.comments.inline.push(comment.value);
}
}
}

// Create an entry for every alias (import) within the statement.
// Create an entry for every alias (member) within the statement.
for alias in names {
let import_from = if alias.name == "*" {
block
.import_from_star
.entry(ImportFromData { module, level })
.or_default()
} else if alias.asname.is_none() || combine_as_imports || force_single_line {
} else if !isolate_aliases && (alias.asname.is_none() || combine_as_imports) {
block
.import_from
.entry(ImportFromData { module, level })
Expand Down Expand Up @@ -127,7 +162,7 @@ pub fn normalize_imports(
}

// Propagate trailing commas.
if matches!(trailing_comma, TrailingComma::Present) {
if !isolate_aliases && matches!(trailing_comma, TrailingComma::Present) {
import_from.trailing_comma = TrailingComma::Present;
}
}
Expand Down
41 changes: 21 additions & 20 deletions ...es/isort/snapshots/ruff__rules__isort__tests__force_single_line_force_single_line.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,37 +41,38 @@ force_single_line.py:1:1: I001 [*] Import block is un-sorted or un-formatted
6 6 | from json import load
7 7 | from json import loads as json_loads
8 |-from logging.handlers import StreamHandler, FileHandler
9 |-
10 |-# comment 1
11 |-from third_party import lib1, lib2, \
12 |- lib3, lib7, lib5, lib6
13 |-# comment 2
14 |-from third_party import lib4
8 |+from logging.handlers import FileHandler, StreamHandler
9 |+from os import path, uname
15 10 |
9 10 |
11 |+# comment 6
12 |+from bar import a # comment 7
13 |+from bar import b # comment 8
16 14 | from foo import bar # comment 3
17 15 | from foo2 import bar2 # comment 4
18 |-from foo3 import bar3, baz3 # comment 5
14 |+from foo import bar # comment 3
15 |+from foo2 import bar2 # comment 4
16 |+from foo3 import bar3 # comment 5
17 |+from foo3 import baz3 # comment 5
19 18 |
18 |+
10 19 | # comment 1
11 |-from third_party import lib1, lib2, \
12 |- lib3, lib7, lib5, lib6
20 |+from third_party import lib1
21 |+from third_party import lib2
22 |+from third_party import lib3
23 |+
13 24 | # comment 2
14 25 | from third_party import lib4
15 |-
16 |-from foo import bar # comment 3
17 |-from foo2 import bar2 # comment 4
18 |-from foo3 import bar3, baz3 # comment 5
19 |-
20 |-# comment 6
21 |-from bar import (
22 |- a, # comment 7
23 |- b, # comment 8
24 |-)
19 |+# comment 1
20 |+# comment 2
21 |+from third_party import lib1
22 |+from third_party import lib2
23 |+from third_party import lib3
24 |+from third_party import lib4
25 |+from third_party import lib5
26 |+from third_party import lib6
27 |+from third_party import lib7
26 |+from third_party import lib5
27 |+from third_party import lib6
28 |+from third_party import lib7


20 changes: 20 additions & 0 deletions ...ts/ruff__rules__isort__tests__propagate_inline_comments_propagate_inline_comments.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
source: crates/ruff/src/rules/isort/mod.rs
---
propagate_inline_comments.py:1:1: I001 [*] Import block is un-sorted or un-formatted
|
1 | / from mypackage.subpackage import ( # long comment that seems to be a problem
2 | | a_long_variable_name_that_causes_problems,
3 | | items,
4 | | )
|
= help: Organize imports

ℹ Suggested fix
1 1 | from mypackage.subpackage import ( # long comment that seems to be a problem
2 2 | a_long_variable_name_that_causes_problems,
3 |- items,
4 3 | )
4 |+from mypackage.subpackage import items # long comment that seems to be a problem