-
Notifications
You must be signed in to change notification settings - Fork 912
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 blank line between match inner attrs and first arm #6119
base: master
Are you sure you want to change the base?
Conversation
* manually find blank line in span between inner attrs and first match arm or inner attrs and comment before match arm * only single blank line is preserved
I haven't reviewed the code yet, but I took a quick look at the test cases. When you have a moment can you add a test case where there's more than one blank line between the inner attrs and match arms. Additionally, can you add a test case where there are no blank lines between the inner attrs and match arms. |
@ytmimi I added test cases for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for another PR. Take a look at the feedback when you get a chance. I think we need to consider a few more cases, and extract the logic for blank_line_after_inner_attrs
into a separate function.
@@ -121,6 +121,30 @@ pub(crate) fn rewrite_match( | |||
inner_attrs[inner_attrs.len() - 1].span.hi() | |||
}; | |||
|
|||
let blank_line_after_inner_attrs = if context.config.version() == Version::Two | |||
&& !inner_attrs.is_empty() | |||
&& !arms.is_empty() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I'm seeing the !arms.is_empty()
condition I think we should also add test cases showing how we reformat a match with inner attributes but no match arms.
let blank_line_snip = if let Some(hi) = context | ||
.snippet(mk_sp(open_brace_pos, arms[0].span.lo())) | ||
.find('/') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably the .find('/')
call is searching for comments? I think it's more appropriate to use the Span
from the last inner attribute instead of the open_brace_pos
. I think using the open_brace_pos
could lead to subtle formatting errors for these cases:
pub fn main() {
match a {
// comment before inner attributes
#![attr1]
#![attr2]
_ => None,
}
match a {
#![attr1]
// comment between inner attributes
#![attr2]
_ => None,
}
}
let blank_line_after_inner_attrs = if context.config.version() == Version::Two | ||
&& !inner_attrs.is_empty() | ||
&& !arms.is_empty() | ||
{ | ||
let blank_line_snip = if let Some(hi) = context | ||
.snippet(mk_sp(open_brace_pos, arms[0].span.lo())) | ||
.find('/') | ||
{ | ||
context.snippet(mk_sp( | ||
open_brace_pos, | ||
open_brace_pos + BytePos::from_usize(hi), | ||
)) | ||
} else { | ||
context.snippet(mk_sp(open_brace_pos, arms[0].span.lo())) | ||
}; | ||
if blank_line_snip.find("\n") != blank_line_snip.rfind("\n") { | ||
"\n" | ||
} else { | ||
"" | ||
} | ||
} else { | ||
"" | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nicer to extract the blank_line_after_inner_attrs
calculation into a standalone function so that rewrite_match
isn't responsible for this logic.
Fixes #6005
Solution
This PR performs manual check for blank line in span between (
inner attrs
andfirst match arm
) or (inner attrs
andcomment
before match arm) and then adds it back.Question