-
Notifications
You must be signed in to change notification settings - Fork 410
Convert file-level rustfmt skip to fn level skips #3809
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
base: main
Are you sure you want to change the base?
Conversation
👋 Hi! I see this is a draft PR. |
9874e55
to
69a9581
Compare
a88062e
to
16025f2
Compare
As discussed in sync meet, I will try to extend the script to ignore rustfmt changes that only affect the function signature. |
16025f2
to
9ff8996
Compare
Pushed fix up commit to ignore sig changes. Modest reduction in skips, with a few edge cases. |
They refer to other files and make rustfmt want to search for those. This is problematic with the skip-insertion script.
Those two directives don't seem to work together properly and make the build fail.
9ff8996
to
cf4e480
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3809 +/- ##
==========================================
+ Coverage 89.73% 89.79% +0.05%
==========================================
Files 159 159
Lines 128910 129545 +635
Branches 128910 129545 +635
==========================================
+ Hits 115676 116323 +647
+ Misses 10536 10533 -3
+ Partials 2698 2689 -9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Why is this in draft? This needs a rebase by now, too.
@@ -1,48 +1,47 @@ | |||
#![cfg_attr(rustfmt, rustfmt_skip)] |
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.
This was put here deliberately as we opted against running rustfmt
on autogenerated files. We need to fix it either way, likely just revert it to include the skip.
Extend the approach proposed in #3767 to the full repository. The change set is automatically generated using a script. The script omits skip directives when they would not impact formatting (code already rustfmt-compliant).
I've fixed various problems with the script, and it may be that there are a few edge cases left that should have had a skip directive. There could also be a few redundant ones. I think we can live with that.