-
Notifications
You must be signed in to change notification settings - Fork 908
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
fix(rustfmt): fix struct field formatting with doc comments present #5217
fix(rustfmt): fix struct field formatting with doc comments present #5217
Conversation
940eaa1
to
302a27c
Compare
302a27c
to
0d15b1f
Compare
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 submitting this PR!
A few minor tweaks, and then I think we should be good to go on this one.
Thanks again for taking on the PR, and for the quick turnaround after receiving some feedback! CI checks look like they're off to a good start, but I'll check back up on this later for another review. |
The code changes look good to me! I'm wondering if we could add another test case around an attribute that isn't necessarily a doc comment. For example, what does this get formatted to? struct MyTuple(
#[cfg(unix)] // some comment
u64,
#[cfg(not(unix))] /*block comment */
u32,
);
struct MyTuple(
#[cfg(unix)]
// some comment
u64,
#[cfg(not(unix))]
/*block comment */
u32,
); |
@tharun208 Thanks again for all the work on this. I know there's a been a decent amount of back and forth so far. For completeness, and to improve the test coverage could you take all of the tuple struct tests and also include regular struct versions of them. For the regular struct versions of the tests could you also add some fields with various visibility modifiers like |
@ytmimi sorry for replying late. will improve the coverage and possibly come with the changes next week. |
@tharun208 absolutely no worries. I totally get that things come up and working on PRs can get away from you. No rush on the requested tests, but if you have time to work on them next that would be great! |
@ytmimi can we merge this and introduce the test changes as a new pr? |
I personally don't have an issue with that, and I think the current tests show that the fix works, but just to give you an example these were the additional test cases that I was looking for: struct MyTuple(
#[cfg(unix)] // some comment
pub u64,
#[cfg(not(unix))] /*block comment */
pub(crate) u32,
);
struct MyTuple(
/// Doc Comments
/* TODO note to add more to Doc Comments */
pub u32,
/// Doc Comments
// TODO note
pub(crate) u64,
);
struct MyStruct {
#[cfg(unix)] // some comment
a: u64,
#[cfg(not(unix))] /*block comment */
b: u32,
}
struct MyStruct {
#[cfg(unix)] // some comment
pub a: u64,
#[cfg(not(unix))] /*block comment */
pub(crate) b: u32,
}
struct MyStruct {
/// Doc Comments
/* TODO note to add more to Doc Comments */
a: u32,
/// Doc Comments
// TODO note
b: u64,
}
struct MyStruct {
/// Doc Comments
/* TODO note to add more to Doc Comments */
pub a: u32,
/// Doc Comments
// TODO note
pub(crate) b: u64,
} |
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.
Thank you for adding those additional test cases!! Just one minor tweak to reintroduce a newline at the of test/souce/structs.rs
, and I think we're good to go here.
@ytmimi done 😃 |
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.
LGTM Thanks for this!
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.
lgtm as well, thank you both!
Fixes #5215