Skip to content

Backport #4572 - Proper indentation of multiline post-comments in a list #5649

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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
20 changes: 19 additions & 1 deletion src/lists.rs
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,13 @@ where

let mut formatted_comment = rewrite_post_comment(&mut item_max_width)?;

// Multiline comments are not included in a previous "indentation group".
// Each multiline comment is considered as a separate group.
if formatted_comment.contains('\n') {
item_max_width = None;
formatted_comment = rewrite_post_comment(&mut item_max_width)?;
}

if !starts_with_newline(comment) {
if formatting.align_comments {
let mut comment_alignment =
Expand Down Expand Up @@ -534,10 +541,21 @@ where
for item in items.clone().into_iter().skip(i) {
let item = item.as_ref();
let inner_item_width = unicode_str_width(item.inner_as_ref());
let post_comment_is_multiline = item
.post_comment
.as_ref()
.map_or(false, |s| s.trim().contains('\n'));

// Each multiline comment is an "indentation group" on its own
if first && post_comment_is_multiline {
return inner_item_width;
}

if !first
&& (item.is_different_group()
|| item.post_comment.is_none()
|| inner_item_width + overhead > max_budget)
|| inner_item_width + overhead > max_budget
|| post_comment_is_multiline)
{
return max_width;
}
Expand Down
163 changes: 163 additions & 0 deletions tests/source/issue-4546.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
// Basic tests (using vectors) - list multiline post-comment is indented on its own
fn main() {
let v = [
"A", /* item A comment */
"BBB", /* item B comment */
"CCCCCC", /* item C comment line 1
* item C comment line 2 */
"D", /* item D comment line 1
* item D comment line 2 */
"EEEEE", /* item E comment */
"FFF", /* item F comment */
"GG", /* item G comment line 1
* item G comment line 2 */
];
}

fn main() {
let v = [
"GG", /* item G comment line 1
* item G comment line 2 */
"AAAAA", /* item A comment */
"BBB", /* item B comment */
"CCCCCC", /* item C comment line 1
* item C comment line 2 */
"D", /* item D comment line 1
* item D comment line 2 */
"E", /* item E comment */
"FFF", /* item F comment */
];
}

// Tests for Struct
pub(crate) struct ListFormatting<'a> {
tactic: DefinitiveListTactic,
separator: &'a str, /* Comment */
trailing_separator: SeparatorTactic, /* Comment */
separator_place: SeparatorPlace, // Comment 1
// Comment 2
shape: Shape, /* Non-expressions, e.g., items, will have a new line at the end of the list.
* Important for comment styles. */
ends_with_newline: bool, // Remove newlines between list elements for expressions.
preserve_newline: bool, // Nested import lists get some special handling for the "Mixed" list type
nested: bool,
// Whether comments should be visually aligned.
align_comments: bool, /* comment */
config: &'a Config,
}

fn main() {
l = ListItem {
pre_comment, /* Comment */
pre_comment_style, /* Multiline comment
* Line 2 */
/* New line comment */
item: if self.inner.peek().is_none() && self.leave_last { /* Comment */
None
} else {
(self.get_item_string)(&item)
},
post_comment, /* Comment */
new_lines, /* Comment */
}
}

// Test for Function parameters
pub(crate) fn new(shape: Shape, config: &'a Config) -> Self {
ListFormatting {
tactic: DefinitiveListTactic::Vertical, /* Comment */
separator: ",", /* comment */
trailing_separator: SeparatorTactic::Never, /* Multiline comment
* second comment line */
separator_place: SeparatorPlace::Back, // A longggggggggggggggggggggggggggggggggggggggggggggggggggg comment
shape,
/* New line comment */
ends_with_newline: true, /* Comment */
preserve_newline: false, /* Comment */
nested: false, /* Comment */
align_comments: true, /* Another Multiline comment
* second comment line */
config, /* Last comment */
}
}

// Test for `where`
impl<'a, T, I, F1, F2, F3> Iterator for ListItems<'a, I, F1, F2, F3>
where
I: Iterator<Item = T>, /* Comment */
F111111111: Fn(&T) -> BytePos, /* Comment */
F2222222: Fn(&T) -> BytePos, /* Multiline comment
* Line 2 */
F3: Fn(&T) -> Option<String>, /* Comment */
{}

// Test for some types of lists
pub(crate) fn itemize_list<'a, T, I, F1, F2, F3>(
snippet_provider: &'a SnippetProvider, /* Comment */
inner: I, /* Comment */
terminator: &'a str, /* Multiline comment
* Line 2 */
separator: &'a str, /* Comment */
get_lo: F1, /* Comment */
get_hi: F2,
get_item_string: F3, /* Comment */
prev_span_end: BytePos, /* Multiline comment
* Line 2 */
next_span_start: BytePos, /* Comment */
leave_last: bool,
) -> ListItems<'a, I, F1, F2, F3>
where
I: Iterator<Item = T>, /* Comment */
F111111111: Fn(&T) -> BytePos, /* Multiline comment
* Line 2 */
F2222222: Fn(&T) -> BytePos,
F3: Fn(&T) -> Option<String>, /* Comment */
{
ListItems {/* Comment to ListItems */
snippet_provider, /* Multiline comment
* Line 2 */
inner: inner.peekable(), /* Another multiline comment
* another Line 2 */
get_lo, /* Comment */
get_hi,
get_item_string,
prev_span_end, /* Comment */
next_span_start,/* Comment */
terminator,
separator, /* Yet another multiline comment
* yet another Line 2 */
leave_last,/* Comment */
}
}


// Tests when comment in the same line of the item will exceed line width
fn main() {
let v = [
"A-Longgggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg", /* item A comment */
"BBB", /* item B Longgggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg comment */
"CCCCCC", /* item C comment line 1
* item C comment line 2 */
"D-Longggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg", /* item D comment line 1
* item D comment line 2 */
"EEEEE", /* item E comment */
];
}

// Test for nested items
fn main() {
let v1 = [
"GG", /* item G comment line 1
* item G comment line 2 */
"AAAAA", /* item A comment */
[
"BBB", /* item B comment */
"CCCCCC", /* item C comment line 1
* item C comment line 2 */
"D", /* item D comment line 1
* item D comment line 2 */
"E", /* item E comment */
],
"FFF", /* item F comment */
];
}
165 changes: 165 additions & 0 deletions tests/target/issue-4546.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,165 @@
// Basic tests (using vectors) - list multiline post-comment is indented on its own
fn main() {
let v = [
"A", /* item A comment */
"BBB", /* item B comment */
"CCCCCC", /* item C comment line 1
* item C comment line 2 */
"D", /* item D comment line 1
* item D comment line 2 */
"EEEEE", /* item E comment */
"FFF", /* item F comment */
"GG", /* item G comment line 1
* item G comment line 2 */
];
}

fn main() {
let v = [
"GG", /* item G comment line 1
* item G comment line 2 */
"AAAAA", /* item A comment */
"BBB", /* item B comment */
"CCCCCC", /* item C comment line 1
* item C comment line 2 */
"D", /* item D comment line 1
* item D comment line 2 */
"E", /* item E comment */
"FFF", /* item F comment */
];
}

// Tests for Struct
pub(crate) struct ListFormatting<'a> {
tactic: DefinitiveListTactic,
separator: &'a str, /* Comment */
trailing_separator: SeparatorTactic, /* Comment */
separator_place: SeparatorPlace, // Comment 1
Comment on lines +35 to +37
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should these comments be aligned?

Copy link
Contributor Author

@davidBar-On davidBar-On Jan 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for the alignment of the one line comments is backward compatibility - see this #4546 comment. The workaround is actually applicable only to one line comments. However, assuming that one line comments are the common case, it should be applicable for most of the cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the link to the previous discussion!

// Comment 2
shape: Shape, /* Non-expressions, e.g., items, will have a new line at the end of the list.
* Important for comment styles. */
ends_with_newline: bool, // Remove newlines between list elements for expressions.
preserve_newline: bool, // Nested import lists get some special handling for the "Mixed" list type
nested: bool,
// Whether comments should be visually aligned.
align_comments: bool, /* comment */
config: &'a Config,
}

fn main() {
l = ListItem {
pre_comment, /* Comment */
pre_comment_style, /* Multiline comment
* Line 2 */
/* New line comment */
item: if self.inner.peek().is_none() && self.leave_last {
/* Comment */
None
} else {
(self.get_item_string)(&item)
},
post_comment, /* Comment */
new_lines, /* Comment */
}
}

// Test for Function parameters
pub(crate) fn new(shape: Shape, config: &'a Config) -> Self {
ListFormatting {
tactic: DefinitiveListTactic::Vertical, /* Comment */
separator: ",", /* comment */
Comment on lines +69 to +70
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, should these comments be aligned?

trailing_separator: SeparatorTactic::Never, /* Multiline comment
* second comment line */
separator_place: SeparatorPlace::Back, // A longggggggggggggggggggggggggggggggggggggggggggggggggggg comment
shape,
/* New line comment */
ends_with_newline: true, /* Comment */
preserve_newline: false, /* Comment */
nested: false, /* Comment */
align_comments: true, /* Another Multiline comment
* second comment line */
config, /* Last comment */
}
}

// Test for `where`
impl<'a, T, I, F1, F2, F3> Iterator for ListItems<'a, I, F1, F2, F3>
where
I: Iterator<Item = T>, /* Comment */
F111111111: Fn(&T) -> BytePos, /* Comment */
F2222222: Fn(&T) -> BytePos, /* Multiline comment
* Line 2 */
F3: Fn(&T) -> Option<String>, /* Comment */
{
}

// Test for some types of lists
pub(crate) fn itemize_list<'a, T, I, F1, F2, F3>(
snippet_provider: &'a SnippetProvider, /* Comment */
inner: I, /* Comment */
terminator: &'a str, /* Multiline comment
* Line 2 */
separator: &'a str, /* Comment */
get_lo: F1, /* Comment */
get_hi: F2,
get_item_string: F3, /* Comment */
prev_span_end: BytePos, /* Multiline comment
* Line 2 */
next_span_start: BytePos, /* Comment */
leave_last: bool,
) -> ListItems<'a, I, F1, F2, F3>
where
I: Iterator<Item = T>, /* Comment */
F111111111: Fn(&T) -> BytePos, /* Multiline comment
* Line 2 */
F2222222: Fn(&T) -> BytePos,
F3: Fn(&T) -> Option<String>, /* Comment */
{
ListItems {
/* Comment to ListItems */
snippet_provider, /* Multiline comment
* Line 2 */
inner: inner.peekable(), /* Another multiline comment
* another Line 2 */
get_lo, /* Comment */
get_hi,
get_item_string,
prev_span_end, /* Comment */
next_span_start, /* Comment */
terminator,
separator, /* Yet another multiline comment
* yet another Line 2 */
leave_last, /* Comment */
}
}

// Tests when comment in the same line of the item will exceed line width
fn main() {
let v = [
"A-Longgggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg", /* item A comment */
"BBB", /* item B Longgggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg comment */
"CCCCCC", /* item C comment line 1
* item C comment line 2 */
"D-Longggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg", /* item D comment line 1
* item D comment line 2 */
"EEEEE", /* item E comment */
];
}

// Test for nested items
fn main() {
let v1 = [
"GG", /* item G comment line 1
* item G comment line 2 */
"AAAAA", /* item A comment */
[
"BBB", /* item B comment */
"CCCCCC", /* item C comment line 1
* item C comment line 2 */
"D", /* item D comment line 1
* item D comment line 2 */
"E", /* item E comment */
],
"FFF", /* item F comment */
];
}