Run rustfmt on `channelmanager` #3767

joostjager · 2025-05-05T07:45:20Z

After reviewing a few rustfmt PRs and observing that personal preferences regarding formatting vary, I tried to do this one in a minimal way.

I went through the diff looking for blow up that really stood out, but there wasn't much that looked outside acceptable bounds to me. If anything, most of what rustfmt does is a positive change the way I see it. Reducing line density helps with readability, for me.

There were two match arms for UpdateAddHTLC that maybe received a bit too much love, even though I think even those can go through as is. Added #[rustfmt::skip] nonetheless to at least avoid what I think would be the worst pain for some.

Outstanding PRs can be rebased painlessly using a variant of the script described in #3749 (comment).

Update 5/27/2025: Added skips to every method.

ldk-reviews-bot · 2025-05-05T07:45:23Z

👋 Thanks for assigning @valentinewallace 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.

ldk-reviews-bot · 2025-05-07T07:54:04Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-05-10T00:00:42Z

🔔 2nd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-05-12T00:01:14Z

🔔 3rd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-05-14T00:01:46Z

🔔 4th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-05-17T00:00:48Z

🔔 5th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-05-19T00:01:17Z

🔔 6th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-05-21T00:01:55Z

🔔 7th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-05-24T00:00:40Z

🔔 8th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-05-26T00:00:51Z

🔔 9th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

joostjager · 2025-05-27T09:41:59Z

Pushed a new take on channelmanager formatting. Used an automated script to insert the skip directives before every function.

codecov · 2025-05-27T09:56:05Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.89%. Comparing base (c6caad8) to head (83f41dd).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3767      +/-   ##
==========================================
+ Coverage   89.87%   89.89%   +0.02%     
==========================================
  Files         160      160              
  Lines      129136   129172      +36     
  Branches   129136   129172      +36     
==========================================
+ Hits       116056   116123      +67     
+ Misses      10381    10358      -23     
+ Partials     2699     2691       -8

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

ldk-reviews-bot · 2025-05-28T00:01:27Z

🔔 10th Reminder

Hey @TheBlueMatt @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

valentinewallace

Concept ACK, I like this as an incremental approach, and any new methods that get added will be rustfmt'd from the start. Browsing through channelmanager.rs, it doesn't really seem too invasive to me.

Curious your thoughts moving forward after this --

Do you plan to create a follow-up removing (many of?) the skips? Or should we have a policy where going forward if a method is touched in a PR then it should be formatted?

lightning/src/ln/channelmanager.rs

joostjager · 2025-05-28T10:31:29Z

Rebased after merge of #3802. Smooth workflow now in channelmanager.rs. Just remove a skip attribute and save (format on save) to go to standard formatting.

joostjager · 2025-05-28T10:37:00Z

Concept ACK, I like this as an incremental approach, and any new methods that get added will be rustfmt'd from the start. Browsing through channelmanager.rs, it doesn't really seem too invasive to me.

It's indeed just the structs and enums that are changing. But those weren't easy to format-fix anyway.

Curious your thoughts moving forward after this --
Do you plan to create a follow-up removing (many of?) the skips? Or should we have a policy where going forward if a method is touched in a PR then it should be formatted?

Good question. If it were up to me, I'd just format the whole repository without any skips, and use the rebase script for outstanding PRs. But as there is no unanimous support for that, I am trying to at least make it better than what it was.

I think going forward from this PR, it's up to contributors to remove skip directives as they see fit. Probably based on functions touched. If that's done in a separate initial commit, and the same is done for conflicting prs that touch the same functions, I think a rebase shouldn't be problematic.

Alternatively a manual pass through this file can be made to remove some of the skips and wrap that in a separate PR. That's probably best done by someone who knows the parts of channelmgr that are in flight.

jkczyz · 2025-05-28T19:39:49Z

Concept ACK. I like this approach. A follow-up PR can probably remove a lot of skips where rustfmt makes trivial changes or those where there's no obvious refactoring. Then -- when touching a function in a PR -- anyone inclined could refactor it and drop the skip on a separate commit.

valentinewallace · 2025-05-28T21:55:51Z

~~I had to add edition = "2021" to our rustfmt.toml for format-on-save to work in channelmanager.rs. Did you experience this @joostjager?~~

~~Without this, cargo fmt also does nothing and rustfmt channelmanager.rs errors with:~~

error[E0670]: `async fn` is not permitted in Rust 2015
     --> /Users/valentine/rust-lightning/lightning/src/ln/channelmanager.rs:11468:6
      |
11468 |     pub async fn process_pending_events_async<
      |         ^^^^^ to use `async fn`, switch to Rust 2018 or later
      |
      = help: pass `--edition 2021` to `rustc`
      = note: for more on editions, read https://doc.rust-lang.org/edition-guide

Edit: fixed this as well, see below comment.

valentinewallace · 2025-05-28T22:38:37Z

FYI, there does seem to be some lag when formatting-on-save. At one point it was extremely slow and the lag was >10 seconds, but I'm unable to reproduce that now. For me the lag doesn't seem like a dealbreaker, but others may want to test it out.

~~Edit: I got some quite slow lag again after the file had been open for a while, restarting my text editor fixed it. Hmm.~~

Edit 2: Never mind, it was due to using a vim global for format-on-save instead of nvim_create_autocmd. Thanks @wpaulino for pointing this out!

valentinewallace

Ship it! 👏

Dismissing my review because there's a fixup commit. But the ACK stands

joostjager · 2025-05-29T04:36:43Z

Squashed

tnull

Concept ACK: I like this approach - whoever touches some code would just remove the corresponding rustfmt::skip, and some time down the line we'd bit the bullet to do the remaining parts in one go (i.e., the parts that nobody ever touches).

joostjager · 2025-05-29T13:47:02Z

Prepared a follow up PR for the full repo: #3809

joostjager · 2025-05-30T08:21:48Z

As discussed in sync meet, I've updated the script to drop the skip directives for functions that only need their signature to change. Added fixup commit.

ldk-reviews-bot · 2025-05-31T00:00:16Z

🔔 11th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-06-02T00:01:26Z

🔔 12th Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

valentinewallace · 2025-06-02T15:16:58Z

Fixup LGTM

This allows devs to enable rustfmt on a per-function level.

joostjager · 2025-06-03T11:56:52Z

Rebased. Would be nice to move towards a conclusion on this approach.

joostjager requested a review from TheBlueMatt May 5, 2025 07:53

joostjager added the rustfmt label May 12, 2025

joostjager force-pushed the fmt-chanmgr branch from ab44c3d to 1d787c6 Compare May 27, 2025 09:39

joostjager requested a review from valentinewallace May 27, 2025 09:42

joostjager force-pushed the fmt-chanmgr branch 2 times, most recently from f033d19 to d7b433e Compare May 27, 2025 09:46

valentinewallace reviewed May 28, 2025

View reviewed changes

lightning/src/ln/channelmanager.rs Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

joostjager force-pushed the fmt-chanmgr branch 6 times, most recently from fd5301e to ae7d478 Compare May 28, 2025 10:29

joostjager requested a review from valentinewallace May 28, 2025 15:25

valentinewallace previously approved these changes May 28, 2025

View reviewed changes

joostjager force-pushed the fmt-chanmgr branch from ae7d478 to 6d8b2b4 Compare May 29, 2025 04:36

joostjager requested a review from valentinewallace May 29, 2025 04:37

tnull reviewed May 29, 2025

View reviewed changes

joostjager mentioned this pull request May 29, 2025

Convert file-level rustfmt skip to fn level skips #3809

Draft

valentinewallace previously approved these changes May 29, 2025

View reviewed changes

joostjager dismissed valentinewallace’s stale review via d49694a May 30, 2025 08:18

joostjager force-pushed the fmt-chanmgr branch 3 times, most recently from 08ccc6f to edf9c57 Compare May 30, 2025 08:20

joostjager force-pushed the fmt-chanmgr branch from edf9c57 to a08cc4e Compare June 2, 2025 13:00

joostjager force-pushed the fmt-chanmgr branch from a08cc4e to 384ca93 Compare June 3, 2025 11:53

Add rustfmt skip directives to channelmanager.rs

83f41dd

This allows devs to enable rustfmt on a per-function level.

joostjager force-pushed the fmt-chanmgr branch from 384ca93 to 83f41dd Compare June 3, 2025 11:56

Run rustfmt on channelmanager #3767

Are you sure you want to change the base?

Run rustfmt on channelmanager #3767

Conversation

joostjager commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented May 7, 2025

Uh oh!

ldk-reviews-bot commented May 10, 2025

Uh oh!

ldk-reviews-bot commented May 12, 2025

Uh oh!

ldk-reviews-bot commented May 14, 2025

Uh oh!

ldk-reviews-bot commented May 17, 2025

Uh oh!

ldk-reviews-bot commented May 19, 2025

Uh oh!

ldk-reviews-bot commented May 21, 2025

Uh oh!

ldk-reviews-bot commented May 24, 2025

Uh oh!

ldk-reviews-bot commented May 26, 2025

Uh oh!

joostjager commented May 27, 2025

Uh oh!

codecov bot commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ldk-reviews-bot commented May 28, 2025

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joostjager commented May 28, 2025

Uh oh!

joostjager commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkczyz commented May 28, 2025

Uh oh!

valentinewallace commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

valentinewallace commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

joostjager commented May 29, 2025

Uh oh!

tnull left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joostjager commented May 29, 2025

Uh oh!

joostjager commented May 30, 2025

Uh oh!

ldk-reviews-bot commented May 31, 2025

Uh oh!

ldk-reviews-bot commented Jun 2, 2025

Uh oh!

valentinewallace commented Jun 2, 2025

Uh oh!

joostjager commented Jun 3, 2025

Uh oh!

Uh oh!

Run rustfmt on `channelmanager` #3767

Run rustfmt on `channelmanager` #3767

joostjager commented May 5, 2025 •

edited

Loading

ldk-reviews-bot commented May 5, 2025 •

edited

Loading

codecov bot commented May 27, 2025 •

edited

Loading

joostjager commented May 28, 2025 •

edited

Loading

valentinewallace commented May 28, 2025 •

edited

Loading

valentinewallace commented May 28, 2025 •

edited

Loading

tnull left a comment •

edited

Loading