Backport PR #4730 #5390
Backport PR #4730 #5390
Conversation
|
@davidBar-On thanks so much for this backport! When you have a chance can you explicitly set |
Done |
src/types.rs
Outdated
| match &**args_in { | ||
| ast::AngleBracketed(args) => { | ||
| if args.args.len() > 1 { | ||
| true | ||
| } else { | ||
| false | ||
| } | ||
| } | ||
| _ => false, | ||
| } |
There was a problem hiding this comment.
I think we could simplify this to:
matches!(
args_in.deref(),
ast::GenericArgs::AngleBracketed(bracket_args) if bracket_args.args.len() > 1
)
Done |
|
If you get a chance can you check to see if this resolves the case @jyn514 mentioned in #4689 (comment) too (and if it does add a test case case for it) 🙏 |
I checked and that issue is not resolved by this PR 😞 |
src/types.rs
Outdated
| let retry_with_force_newline = | ||
| if force_newline || (!result.0.contains('\n') && result.0.len() <= shape.width) { | ||
| false | ||
| } else { | ||
| if items.len() > 1 { | ||
| true | ||
| } else { | ||
| match items[0] { | ||
| ast::GenericBound::Trait(ref poly_trait_ref, ..) => { | ||
| let segments = &poly_trait_ref.trait_ref.path.segments; | ||
| if segments.len() > 1 { | ||
| true | ||
| } else { | ||
| is_segment_with_multi_items_array(&segments[0]) | ||
| } | ||
| } | ||
| _ => false, | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
I wonder if it's worth pulling this logic out into a function or a closure to help with the indentation. Something like this:
fn retry_with_force_newline(
items: &[ast::GenericBound],
force_newline: bool,
result: &str,
shape: Shape,
) -> bool {
if force_newline || (!result.contains('\n') && result.len() <= shape.width) {
return false;
}
if items.len() > 1 {
return true;
}
if let ast::GenericBound::Trait(ref poly_trait_ref, ..) = items[0] {
let segments = &poly_trait_ref.trait_ref.path.segments;
if segments.len() > 1 {
true
} else {
is_segment_with_multi_items_array(&segments[0])
}
} else {
false
}
}There was a problem hiding this comment.
Pulled some of the logic into is_segment_with_multi_items_array closure and renamed it to is_item_with_multi_items_array, which I believe is good enough.
Originally, is_segment_with_multi_items_array was specified to reduce indentations. Because of a previous comment about using matches! instead of match, the indentations in the closure were reduces, so it was possible to add more logic into the closure.
| // Some test cases to ensure other cases formatting were not changed | ||
| fn f() -> Box< | ||
| FnMut() -> Thing< | ||
| WithType = LongItemName, | ||
| Error = LONGLONGLONGLONGLONGONGEvenLongerErrorNameLongerLonger, | ||
| >, | ||
| > { | ||
| } | ||
| fn f() -> Box< | ||
| FnMut() -> Thing< | ||
| WithType = LongItemName, | ||
| Error = LONGLONGLONGLONGLONGONGEvenLongerErrorNameLongerLonger, | ||
| > + fmt::Write1 | ||
| + fmt::Write2, | ||
| > { | ||
| } |
There was a problem hiding this comment.
At first glance this formatting in the 2nd case looks off to me, but if the comment is correct and that's the intended formatting then we're good to go
There was a problem hiding this comment.
As written in the comment before the test cases, these cases were added just to make sure that there is no change to the rustfmt formatting because of the PR changes. Formatting of these test cases was not changed. If the formatting is wrong and it may be changed later, then these test cases may be removed.
| fn foo<F>(foo2: F) | ||
| where | ||
| F: Fn( | ||
| // this comment is deleted | ||
| ), | ||
| { | ||
| } | ||
| fn foo<F>(foo2: F) | ||
| where | ||
| F: Fn( | ||
| // this comment is deleted | ||
| ) + fmt::Write, | ||
| { | ||
| } |
There was a problem hiding this comment.
Same comment as above. It does feel odd that including an additional bound with + would throw off the indentation.
There was a problem hiding this comment.
Same as above. Formatting of these test cases was not changed by this PR and they may be removed if their formatting may be changed later.
d8d6019 to
bb16e04
Compare
|
Rebased |
|
Thanks for backporting this! |
|
Circling back to this as part of prepping release notes, but this doesn't seem to have actually applied a version gate and is thus introducing breaking formatting changes. Lmk if I'm simply mistaken or overlooking something, but this should be directly observable in a comparison between the Playground and the latest on This needs to be addressed ASAP, either via the addition of version gating or by reverting the associated commit(s) cc @ytmimi |
Backport PR #4730 that fix issue #4689.
Test passes with
version = "Two"and verified that no additional test issues are added when version is set to "One".