Convert file-level rustfmt skip to fn level skips

[joostjager]

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.

Omitted are files that are formatted in in-flight rustfmt PRs: #3783, #3725 , #3338 and #3337

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[joostjager]

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

[joostjager]

Pushed fix up commit to ignore sig changes. Modest reduction in skips, with a few edge cases.

[codecov]

Codecov Report

Attention: Patch coverage is 99.01153% with 6 lines in your changes missing coverage. Please review.

Project coverage is 89.91%. Comparing base (e797180) to head (a38db49).

Files with missing lines	Patch %	Lines
lightning/src/chain/channelmonitor.rs	91.17%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3809      +/-   ##
==========================================
+ Coverage   89.87%   89.91%   +0.03%     
==========================================
  Files         162      162              
  Lines      130214   130684     +470     
  Branches   130214   130684     +470     
==========================================
+ Hits       117034   117508     +474     
+ Misses      10486    10484       -2     
+ Partials     2694     2692       -2     

☔ 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.

[tnull]

Why is this in draft? This needs a rebase by now, too.

[tnull]

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.

[joostjager]

Ah, didn't know it was auto-generated. Removed the commit that formats this file.

[joostjager]

It's in draft because I want to see #3767 merged first. Don't want to spend time on rebase if we don't go this way.

[joostjager]

#3767 seems a positive change. New code being added in PRs (for example async payments) is now automatically formatted and not creating more formatting debt. No bad rebase conflicts that I've heard of either.

Enough to go ahead and re-run the script on the latest main.

[tnull]

LGTM from what I've seen so far.

Mind including the same changes for the remaining files in chain,ln, and routing, too?

[joostjager]

LGTM from what I've seen so far.

Mind including the same changes for the remaining files in chain,ln, and routing, too?

Intentionally left these out because there are fmt prs in flight for those files. Links in pr desc

[tnull]

Intentionally left these out because there are fmt prs in flight for those files. Links in pr desc

Ah, right. The remaining ones in routing never went far, so now closed them for now. Please include routing here.

[joostjager]

Re-added routing files.

[TheBlueMatt]

I admit I'm kinda meh oh doing this for everything. There's a handful of files left that are pretty small and can be easily formatted (at least once trampoline PRs land for chain and the router module PRs get picked back up), and for tests I understood the plan to be "move forward, get there soon" (I know, I know, I plan on finishing #3783 this week), so applying skips to everything in tests (which generally aren't regularly touched) just to format them soon seems kinda weird?

For large files that we want to format iteratively (channelmanager, channel, maybe some of chain though many of those files aren't that large, just very actively being edited), this approach makes a ton of sense, for every file I'm not sure that we want to always format iteratively?

[joostjager]

I admit I'm kinda meh oh doing this for everything. There's a handful of files left that are pretty small and can be easily formatted (at least once trampoline PRs land for chain and the router module PRs get picked back up), and for tests I understood the plan to be "move forward, get there soon" (I know, I know, I plan on finishing #3783 this week), so applying skips to everything in tests (which generally aren't regularly touched) just to format them soon seems kinda weird?

I see what you mean. Applying skips now and removing them "soon" feels unnecessary. The way I see it though is that "soon" may be quite a bit further out than we'd like. Trampoline for example may take a while. Adding the skips doesn't hurt, they can easily be removed again if someone feels inclined to format a full file, and they add flexibility for devs to selectively format for whatever reason.

For large files that we want to format iteratively (channelmanager, channel, maybe some of chain though many of those files aren't that large, just very actively being edited), this approach makes a ton of sense, for every file I'm not sure that we want to always format iteratively?

I needed to rebase this PR anyway, and while I did I left out the test files. If you feel it is still too much, maybe you can list the exact files that are definitely good for the incremental approach?

[joostjager]

Also not sure at all that doing the tests all in one with mass edits is ideal: #3783 (review)

[TheBlueMatt]

I think the file list here makes sense. I thought there were substantially more changes to onion_payment.rs for trampoline to come but @valentinewallace tells me they're more in outbound_payment.rs which has more active development and thus higher cost to delaying pushing the format skips down in it.

[TheBlueMatt]

Do you want to go ahead and break the hex constants out to give them names?

[joostjager]

Done. All gave them the same hex name though...

[joostjager]

Removed redundant skips. Didn't do the optional improvements (format short fns), because it is probably easiest to keep this big PR "pure" and do the rest in follow ups.

[joostjager]

Didn't squash the redundant skip removal commit, in case I need to re-run the script and re-apply the edge case fixes.

[TheBlueMatt]

11 files changed, 1984 insertions(+), 668 deletions(-)

Holy diff-size batman!