Skip to content

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

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

Merged
merged 2 commits into from
Dec 19, 2020

Conversation

davidBar-On
Copy link
Contributor

@davidBar-On davidBar-On commented Dec 2, 2020

Fix for issue #4546 to properly indent multiline post-comments in a list.
The approach taken is that each multiline comment is considered a "comments indentation group" of its own, so each multiline comment is indented per the list item it belongs to and is not dependent on other list items` comments. This makes sure that the internal comment lines are properly indented.

Multiline comments already terminates the indentation-group before this change. Therefore assuming that in any case using multiline comments for list items is relatively rare, it is expected that this change will not cause major backward compatibility issues.

@calebcartwright
Copy link
Member

By any chance do you have a feel for if/how this may compound the current bug that's causing comment alignment to be done visually/vertically (refs #4108)?

I know this PR is primarily targetting at addressing indentation of true multi-line comments, but #4108 is an annoyingly persistent bug and a clear divergence from the style guide/formatting principles of rustfmt and want to avoid adding anything that may make that bug harder to fix

@davidBar-On
Copy link
Contributor Author

By any chance do you have a feel for if/how this may compound the current bug that's causing comment alignment to be done visually/vertically (refs #4108)?

The change is highly related to #4018. Currently post-comments to consecutive list items are "grouped", where a group is terminated if a list item does not have a post-comment or have a multi-line post-comment. All the post-comments in a group are visually indented to the same column.

The change I did is that a multi-line comment is regarded as a group of its own, instead of being the last in the previous group.

This change can be modified so that each post-comment of a list item will be regarded as a group of its own, not just multi-line comments, practically solving issue #4018. If desired, enhanced modification is that one-line comments will be visually grouped only when rustfmt-indent_style: Visual is set.

Should I modify the change to one of these two options?

@calebcartwright
Copy link
Member

Gotcha, thanks for the extra info. We definitely need to get #4108 fixed but let's not do it here. Even though #4108 is a bug, the resolution is likely going to be pretty disruptive on the community in that'll probably break a lot of existing formatting.

I need to think through a communication strategy on that before we make that change

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

Changes generally LGTM. Would you mind adding some additional test scenarios? This is a widely used code path, so would be good to get some scenarios that covered attrs, struct members, calls, etc. Also do some with nesting/different levels of indentation and some that run up to the max width limit

Comment on lines 464 to 465
// Mmultiline comments are not included in a previous "indentation group".
// Each multiline comment is a considered as a separage group.
Copy link
Member

Choose a reason for hiding this comment

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

Minor typos

Suggested change
// Mmultiline comments are not included in a previous "indentation group".
// Each multiline comment is a considered as a separage group.
// Multiline comments are not included in a previous "indentation group".
// Each multiline comment is a considered as a separate group.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (with removing the first "a" in the second line)

let post_comment_is_multiline = item
.post_comment
.as_ref()
.map_or(false, |s| s.trim().contains('\n'));
Copy link
Member

Choose a reason for hiding this comment

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

kind of a bummer to have to do the multi vs. single line check multiple times, but at first glance I don't see a good way of avoiding that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried to find solution, but one of the checks whether the comment is mutliline is done in a loop for identifying the group, and the other is done later when formatting each items. Adding post_comment_is_multilne field to ListItem and initialize it in Iterator for ListItem may work, but I didn't try to do such change.

@davidBar-On
Copy link
Contributor Author

Added some test cases.

When adding the long items/comments test, I noted that a list item with a comment may exceed maximum line width (not because of this PR). I didn't try to fix that, and it should probably be a new issue. (Just a note: possible resolution may be to store the span of the post_comment in ListItem instead of storing the snippet, and use combine_strs_with_missing_comments() to add the comment to the list item. Another option is to create combine_strs_with_comments_str() function out of combine_strs_with_missing_comments(), to have a common function to combine comments strings.)

@calebcartwright
Copy link
Member

Thanks!

@calebcartwright calebcartwright merged commit d2d3712 into rust-lang:master Dec 19, 2020
@calebcartwright
Copy link
Member

When adding the long items/comments test, I noted that a list item with a comment may exceed maximum line width (not because of this PR). I didn't try to fix that, and it should probably be a new issue.

I expected that would be the case, and I'd say it's largely to be expected. Long/complex trailing comments (particularly multiline ones) are very uncommon and often problematic for precisely this reason.

In these circumstances it's probably best to defer to the developer with various config options (like error_on_unformatted and/or wrap_comments) instead of adding additional complexity to the code here

davidBar-On added a commit to davidBar-On/rustfmt that referenced this pull request Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants