Run rustfmt on channelmanager
#3767
Conversation
|
👋 Thanks for assigning @valentinewallace as a reviewer! |
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 8th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 9th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
Pushed a new take on channelmanager formatting. Used an automated script to insert the skip directives before every function. |
joostjager
force-pushed
the
fmt-chanmgr
branch
2 times, most recently
from
f033d19 to
d7b433e
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3767 +/- ##
==========================================
- Coverage 89.90% 89.90% -0.01%
==========================================
Files 160 160
Lines 129222 129257 +35
Branches 129222 129257 +35
==========================================
+ Hits 116182 116207 +25
- Misses 10348 10357 +9
- Partials 2692 2693 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🔔 10th Reminder Hey @TheBlueMatt @valentinewallace! This PR has been waiting for your review. |
There was a problem hiding this comment.
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?
joostjager
force-pushed
the
fmt-chanmgr
branch
6 times, most recently
from
fd5301e to
ae7d478
Compare
|
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. |
It's indeed just the structs and enums that are changing. But those weren't easy to format-fix anyway.
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. |
|
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. |
|
Prepared a follow up PR for the full repo: #3809 |
joostjager
force-pushed
the
fmt-chanmgr
branch
3 times, most recently
from
08ccc6f to
edf9c57
Compare
|
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. |
|
🔔 11th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 12th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
Fixup LGTM |
joostjager
force-pushed
the
fmt-chanmgr
branch
2 times, most recently
from
384ca93 to
83f41dd
Compare
|
Rebased. Would be nice to move towards a conclusion on this approach. |
|
🔔 13th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
Rebased |
This allows devs to enable rustfmt on a per-function level.
joostjager
force-pushed
the
fmt-chanmgr
branch
3 times, most recently
from
3c9a1b5 to
28116be
Compare
|
I heard from @joostjager that @TheBlueMatt is okay with landing this. Seems legit! Landing! |
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.