Skip to content

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

joostjager
Copy link
Contributor

@joostjager joostjager commented May 29, 2025

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.

@ldk-reviews-bot
Copy link

👋 Hi! I see this is a draft PR.
I'll wait to assign reviewers until you mark it as ready for review.
Just convert it out of draft status when you're ready for review!

@joostjager joostjager force-pushed the rustfmt-skip-all branch 2 times, most recently from a88062e to 16025f2 Compare May 29, 2025 14:26
@joostjager
Copy link
Contributor Author

As discussed in sync meet, I will try to extend the script to ignore rustfmt changes that only affect the function signature.

@joostjager
Copy link
Contributor Author

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.
Copy link

codecov bot commented Jun 2, 2025

Codecov Report

Attention: Patch coverage is 92.14929% with 61 lines in your changes missing coverage. Please review.

Project coverage is 89.79%. Comparing base (8aae34e) to head (cf4e480).

Files with missing lines Patch % Lines
lightning/src/ln/peer_handler.rs 67.53% 25 Missing ⚠️
lightning/src/routing/utxo.rs 31.25% 11 Missing ⚠️
lightning/src/routing/scoring.rs 69.69% 10 Missing ⚠️
lightning/src/routing/router.rs 71.42% 8 Missing ⚠️
lightning/src/chain/channelmonitor.rs 91.89% 6 Missing ⚠️
lightning/src/lib.rs 75.00% 0 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@tnull tnull left a 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)]
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants