Fix comments rewritten too long#6802
Open
matthewhughes934 wants to merge 2 commits intorust-lang:mainfrom
Open
Fix comments rewritten too long#6802matthewhughes934 wants to merge 2 commits intorust-lang:mainfrom
matthewhughes934 wants to merge 2 commits intorust-lang:mainfrom
Conversation
By replacing the code quoted from the issue with a single comment that more directly exposes the issue. This is to 1. Make the underlying issue easier to reason about 2. Make this test not behave upon the behaviour of indentation length when rewriting lines (my main motivation for this change, since it makes the followup patch simpler) This was tested by partially reverting 368a9b7 with the patch: diff --git i/src/string.rs w/src/string.rs index 3b97118..561e70ac 100644 --- i/src/string.rs +++ w/src/string.rs @@ -278,9 +278,6 @@ fn break_string(max_width: usize, trim_end: bool, line_end: &str, input: &[&str] } cur_index }; - if max_width_index_in_input == 0 { - return SnippetState::EndOfInput(input.concat()); - } // Find the position in input for breaking the string if line_end.is_empty() The re-running tests, or executing `rustmft` directly on this test, triggered the panic from the original bug.
In the case where `comment_width` is less than `max_width` and we want to wrap right on `comment_width`: `rustfmt` would consider it's wrapping length to be exactly `comment_width`, ignoring the length of a trailing indent. The fix is essentially how similar formatting code behaves, see e.g. `shape::Shape::comment`. This involved rewriting some existing tests, without impacting what they're meant to be asserting on. This emphasises that this patch _is not_ backwards compatible: while I believe the change to be correct (some tests had comments longer than `comment_width` that weren't being formatted). So there's a trade-off to be made between correctness and stability, since `comment_width` is still not stabilised this may be acceptable. Issue: rust-lang#6801
cecee09 to
9b9090f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Simplify test case for issue 5023
By replacing the code quoted from the issue with a single comment that
more directly exposes the issue. This is to
when rewriting lines (my main motivation for this change, since it
makes the followup patch simpler)
This was tested by partially reverting
368a9b7 with the patch:
The re-running tests, or executing
rustmftdirectly on this test,triggered the panic from the original bug.
Fix wrapping of comments sometimes making too long lines
In the case where
comment_widthis less thanmax_widthand we wantto wrap right on
comment_width:rustfmtwould consider it's wrappinglength to be exactly
comment_width, ignoring the length of a trailingindent. The fix is essentially how similar formatting code behaves, see
e.g.
shape::Shape::comment.This involved rewriting some existing tests, without impacting what
they're meant to be asserting on. This emphasises that this patch is
not backwards compatible: while I believe the change to be correct
(some tests had comments longer than
comment_widththat weren't beingformatted). So there's a trade-off to be made between correctness and
stability, since
comment_widthis still not stabilised this may beacceptable.
Issue: wrap_comments partially ignores comment_width with bullet lists #6801