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

Preserve trailing inline comments on import-from statements #12498

Merged
merged 1 commit into from
Jul 25, 2024
Merged
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
from mylib import (
MyClient,
MyMgmtClient,
) # some comment

pass

from mylib import (
MyClient,
MyMgmtClient,
); from foo import (
bar
)# some comment

pass

from foo import (
bar
)
from mylib import (
MyClient,
MyMgmtClient,
# some comment
)

pass

from mylib import (
MyClient,
# some comment
)

pass

from mylib import (
MyClient
# some comment
)

pass

from mylib import (
# some comment
MyClient
)

pass

# a
from mylib import ( # b
# c
MyClient # d
# e
) # f
33 changes: 32 additions & 1 deletion crates/ruff_linter/src/rules/isort/annotate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ pub(crate) fn annotate_imports<'a>(
}

// Capture names.
let aliases = names
let mut aliases: Vec<_> = names
.iter()
.map(|alias| {
// Find comments above.
Expand All @@ -112,10 +112,40 @@ pub(crate) fn annotate_imports<'a>(
asname: alias.asname.as_ref().map(|asname| locator.slice(asname)),
atop: alias_atop,
inline: alias_inline,
trailing: vec![],
}
})
.collect();

// Capture trailing comments on the _last_ alias, as in:
// ```python
// from foo import (
// bar,
// # noqa
// )
// ```
if let Some(last_alias) = aliases.last_mut() {
while let Some(comment) =
comments_iter.next_if(|comment| comment.start() < import.end())
{
last_alias.trailing.push(comment);
}
}

// Capture trailing comments, as in:
// ```python
// from foo import (
// bar,
// ) # noqa
// ```
let mut trailing = vec![];
let import_line_end = locator.line_end(import.end());
while let Some(comment) =
comments_iter.next_if(|comment| comment.start() < import_line_end)
{
trailing.push(comment);
}

AnnotatedImport::ImportFrom {
module: module.as_ref().map(|module| locator.slice(module)),
names: aliases,
Expand All @@ -127,6 +157,7 @@ pub(crate) fn annotate_imports<'a>(
},
atop,
inline,
trailing,
}
}
_ => panic!("Expected Stmt::Import | Stmt::ImportFrom"),
Expand Down
69 changes: 55 additions & 14 deletions crates/ruff_linter/src/rules/isort/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@ use ruff_python_codegen::Stylist;

use crate::line_width::{LineLength, LineWidthBuilder};

use super::types::{AliasData, CommentSet, ImportFromData, Importable};
use super::types::{AliasData, ImportCommentSet, ImportFromCommentSet, ImportFromData, Importable};

// Guess a capacity to use for string allocation.
const CAPACITY: usize = 200;

/// Add a plain import statement to the [`RopeBuilder`].
pub(crate) fn format_import(
alias: &AliasData,
comments: &CommentSet,
comments: &ImportCommentSet,
is_first: bool,
stylist: &Stylist,
) -> String {
Expand Down Expand Up @@ -43,8 +43,8 @@ pub(crate) fn format_import(
#[allow(clippy::too_many_arguments)]
pub(crate) fn format_import_from(
import_from: &ImportFromData,
comments: &CommentSet,
aliases: &[(AliasData, CommentSet)],
comments: &ImportFromCommentSet,
aliases: &[(AliasData, ImportFromCommentSet)],
line_length: LineLength,
indentation_width: LineWidthBuilder,
stylist: &Stylist,
Expand All @@ -68,12 +68,19 @@ pub(crate) fn format_import_from(
return single_line;
}

// We can only inline if none of the aliases have atop or inline comments.
// We can only inline if none of the aliases have comments.
if !trailing_comma
&& (aliases.len() == 1
|| aliases
.iter()
.all(|(_, CommentSet { atop, inline })| atop.is_empty() && inline.is_empty()))
|| aliases.iter().all(
|(
_,
ImportFromCommentSet {
atop,
inline,
trailing,
},
)| atop.is_empty() && inline.is_empty() && trailing.is_empty(),
))
&& (!force_wrap_aliases
|| aliases.len() == 1
|| aliases.iter().all(|(alias, _)| alias.asname.is_none()))
Expand All @@ -99,8 +106,8 @@ pub(crate) fn format_import_from(
/// This method assumes that the output source code is syntactically valid.
fn format_single_line(
import_from: &ImportFromData,
comments: &CommentSet,
aliases: &[(AliasData, CommentSet)],
comments: &ImportFromCommentSet,
aliases: &[(AliasData, ImportFromCommentSet)],
is_first: bool,
stylist: &Stylist,
indentation_width: LineWidthBuilder,
Expand All @@ -122,7 +129,7 @@ fn format_single_line(
output.push_str(" import ");
line_width = line_width.add_width(5).add_str(&module_name).add_width(8);

for (index, (AliasData { name, asname }, comments)) in aliases.iter().enumerate() {
for (index, (AliasData { name, asname }, _)) in aliases.iter().enumerate() {
if let Some(asname) = asname {
output.push_str(name);
output.push_str(" as ");
Expand All @@ -136,16 +143,39 @@ fn format_single_line(
output.push_str(", ");
line_width = line_width.add_width(2);
}
}

for comment in &comments.inline {
output.push(' ');
output.push(' ');
output.push_str(comment);
line_width = line_width.add_width(2).add_str(comment);
}

for (_, comments) in aliases {
for comment in &comments.atop {
output.push(' ');
output.push(' ');
output.push_str(comment);
line_width = line_width.add_width(2).add_str(comment);
}

for comment in &comments.inline {
output.push(' ');
output.push(' ');
output.push_str(comment);
line_width = line_width.add_width(2).add_str(comment);
}

for comment in &comments.trailing {
output.push(' ');
output.push(' ');
output.push_str(comment);
line_width = line_width.add_width(2).add_str(comment);
}
}

for comment in &comments.inline {
for comment in &comments.trailing {
output.push(' ');
output.push(' ');
output.push_str(comment);
Expand All @@ -160,8 +190,8 @@ fn format_single_line(
/// Format an import-from statement in multi-line format.
fn format_multi_line(
import_from: &ImportFromData,
comments: &CommentSet,
aliases: &[(AliasData, CommentSet)],
comments: &ImportFromCommentSet,
aliases: &[(AliasData, ImportFromCommentSet)],
is_first: bool,
stylist: &Stylist,
) -> String {
Expand Down Expand Up @@ -208,9 +238,20 @@ fn format_multi_line(
output.push_str(comment);
}
output.push_str(&stylist.line_ending());

for comment in &comments.trailing {
output.push_str(stylist.indentation());
output.push_str(comment);
output.push_str(&stylist.line_ending());
}
}

output.push(')');

for comment in &comments.trailing {
output.push_str(" ");
output.push_str(comment);
}
output.push_str(&stylist.line_ending());

output
Expand Down
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/rules/isort/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ pub(crate) struct AnnotatedAliasData<'a> {
pub(crate) asname: Option<&'a str>,
pub(crate) atop: Vec<Comment<'a>>,
pub(crate) inline: Vec<Comment<'a>>,
pub(crate) trailing: Vec<Comment<'a>>,
}

#[derive(Debug)]
Expand All @@ -56,6 +57,7 @@ pub(crate) enum AnnotatedImport<'a> {
level: u32,
atop: Vec<Comment<'a>>,
inline: Vec<Comment<'a>>,
trailing: Vec<Comment<'a>>,
trailing_comma: TrailingComma,
},
}
Expand Down Expand Up @@ -342,6 +344,7 @@ mod tests {
#[test_case(Path::new("sort_similar_imports.py"))]
#[test_case(Path::new("split.py"))]
#[test_case(Path::new("star_before_others.py"))]
#[test_case(Path::new("trailing_comment.py"))]
#[test_case(Path::new("trailing_suffix.py"))]
#[test_case(Path::new("two_space.py"))]
#[test_case(Path::new("type_comments.py"))]
Expand Down
13 changes: 11 additions & 2 deletions crates/ruff_linter/src/rules/isort/normalize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ pub(crate) fn normalize_imports<'a>(
level,
atop,
inline,
trailing,
trailing_comma,
} => {
// Whether to track each member of the import as a separate entry.
Expand Down Expand Up @@ -80,10 +81,13 @@ pub(crate) fn normalize_imports<'a>(
}
}

// Replicate the inline comments onto every member.
// Replicate the inline (and after) comments onto every member.
for comment in &inline {
import_from.comments.inline.push(comment.value.clone());
}
for comment in &trailing {
import_from.comments.trailing.push(comment.value.clone());
}
}
} else {
if let Some(alias) = names.first() {
Expand Down Expand Up @@ -113,10 +117,12 @@ pub(crate) fn normalize_imports<'a>(
for comment in atop {
import_from.comments.atop.push(comment.value);
}

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

Expand Down Expand Up @@ -161,6 +167,9 @@ pub(crate) fn normalize_imports<'a>(
for comment in alias.inline {
comment_set.inline.push(comment.value);
}
for comment in alias.trailing {
comment_set.trailing.push(comment.value);
}

// Propagate trailing commas.
if !isolate_aliases && matches!(trailing_comma, TrailingComma::Present) {
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/rules/isort/order.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use itertools::Itertools;
use super::settings::Settings;
use super::sorting::{MemberKey, ModuleKey};
use super::types::EitherImport::{self, Import, ImportFrom};
use super::types::{AliasData, CommentSet, ImportBlock, ImportFromStatement};
use super::types::{AliasData, ImportBlock, ImportFromCommentSet, ImportFromStatement};

pub(crate) fn order_imports<'a>(
block: ImportBlock<'a>,
Expand Down Expand Up @@ -49,7 +49,7 @@ pub(crate) fn order_imports<'a>(
.sorted_by_cached_key(|(alias, _)| {
MemberKey::from_member(alias.name, alias.asname, settings)
})
.collect::<Vec<(AliasData, CommentSet)>>(),
.collect::<Vec<(AliasData, ImportFromCommentSet)>>(),
)
},
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,8 @@ as_imports_comments.py:1:1: I001 [*] Import block is un-sorted or un-formatted
12 |-
13 |-from bop import ( # Comment on `bop`
14 |- Member # Comment on `Member`
4 |+from baz import Member as Alias # Comment on `Alias` # Comment on `baz`
5 |+from bop import Member # Comment on `Member` # Comment on `bop`
4 |+from baz import Member as Alias # Comment on `baz` # Comment on `Alias`
5 |+from bop import Member # Comment on `bop` # Comment on `Member`
6 |+from foo import ( # Comment on `foo`
7 |+ Member as Alias, # Comment on `Alias`
15 8 | )


Loading
Loading