Skip to content

Run rustfmt on channelmanager - #3767

Merged
valentinewallace merged 1 commit into
lightningdevkit:mainfrom
joostjager:fmt-chanmgr
Jun 4, 2025
Merged

Run rustfmt on channelmanager#3767
valentinewallace merged 1 commit into
lightningdevkit:mainfrom
joostjager:fmt-chanmgr

Conversation

@joostjager

@joostjagerjoostjager commented May 5, 2025•
edited
Loading

Copy link
Copy Markdown
Contributor

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

ldk-reviews-bot commented May 5, 2025•
edited
Loading

Copy link
Copy Markdown

👋 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

ldk-reviews-bot commented May 7, 2025

Copy link
Copy Markdown

🔔 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

ldk-reviews-bot commented May 10, 2025

Copy link
Copy Markdown

🔔 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

ldk-reviews-bot commented May 12, 2025

Copy link
Copy Markdown

🔔 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

ldk-reviews-bot commented May 14, 2025

Copy link
Copy Markdown

🔔 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

ldk-reviews-bot commented May 17, 2025

Copy link
Copy Markdown

🔔 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

ldk-reviews-bot commented May 19, 2025

Copy link
Copy Markdown

🔔 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

ldk-reviews-bot commented May 21, 2025

Copy link
Copy Markdown

🔔 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

ldk-reviews-bot commented May 24, 2025

Copy link
Copy Markdown

🔔 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

ldk-reviews-bot commented May 26, 2025

Copy link
Copy Markdown

🔔 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

joostjager commented May 27, 2025

Copy link
Copy Markdown
ContributorAuthor

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

@joostjager
joostjagerforce-pushed the fmt-chanmgr branch 2 times, most recently from f033d19 to d7b433eCompareMay 27, 2025 09:46
@codecov

codecovBot commented May 27, 2025•
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.90%. Comparing base (901f72d) to head (3c9a1b5).
Report is 2 commits behind head on main.

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

ldk-reviews-bot commented May 28, 2025

Copy link
Copy Markdown

🔔 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
valentinewallace reviewed May 28, 2025
View reviewed changes

@valentinewallacevalentinewallace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs Outdated
@joostjager
joostjagerforce-pushed the fmt-chanmgr branch 6 times, most recently from fd5301e to ae7d478CompareMay 28, 2025 10:29
@joostjager

joostjager commented May 28, 2025

Copy link
Copy Markdown
ContributorAuthor

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

joostjager commented May 28, 2025•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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

jkczyz commented May 28, 2025

Copy link
Copy Markdown
Contributor

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
valentinewallace previously approved these changes May 29, 2025
View reviewed changes
@joostjager
joostjagerforce-pushed the fmt-chanmgr branch 3 times, most recently from 08ccc6f to edf9c57CompareMay 30, 2025 08:20
@joostjager

joostjager commented May 30, 2025

Copy link
Copy Markdown
ContributorAuthor

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

ldk-reviews-bot commented May 31, 2025

Copy link
Copy Markdown

🔔 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

ldk-reviews-bot commented Jun 2, 2025

Copy link
Copy Markdown

🔔 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

valentinewallace commented Jun 2, 2025

Copy link
Copy Markdown
Contributor

Fixup LGTM

@joostjager
joostjagerforce-pushed the fmt-chanmgr branch 2 times, most recently from 384ca93 to 83f41ddCompareJune 3, 2025 11:56
@joostjager

joostjager commented Jun 3, 2025

Copy link
Copy Markdown
ContributorAuthor

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

@ldk-reviews-bot

ldk-reviews-bot commented Jun 4, 2025

Copy link
Copy Markdown

🔔 13th 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.

tnull
tnull reviewed Jun 4, 2025
View reviewed changes

@tnulltnull left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems this needs yet another rebase unfortunately.

@joostjager

joostjager commented Jun 4, 2025

Copy link
Copy Markdown
ContributorAuthor

Rebased

valentinewallace
valentinewallace previously approved these changes Jun 4, 2025
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs Outdated
@joostjager
Add rustfmt skip directives to channelmanager.rs
28116be
This allows devs to enable rustfmt on a per-function level.
@joostjager
joostjagerforce-pushed the fmt-chanmgr branch 3 times, most recently from 3c9a1b5 to 28116beCompareJune 4, 2025 15:30
@valentinewallace

valentinewallace commented Jun 4, 2025

Copy link
Copy Markdown
Contributor

I heard from @joostjager that @TheBlueMatt is okay with landing this. Seems legit! Landing!

@valentinewallace
valentinewallace merged commit 7b62ea4 into lightningdevkit:mainJun 4, 2025
This was referenced Jun 4, 2025
Merged
Merged
@valentinewallacevalentinewallace mentioned this pull request Jun 12, 2025
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tnulltnulltnull left review comments
@valentinewallacevalentinewallacevalentinewallace approved these changes
@TheBlueMattTheBlueMattAwaiting requested review from TheBlueMatt

Assignees

No one assigned

Labels

rustfmt

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@joostjager@ldk-reviews-bot@jkczyz@valentinewallace@tnull
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Run rustfmt on `channelmanager` by joostjager · Pull Request #3767 · lightningdevkit/rust-lightning · GitHub
Skip to content
Run rustfmt on channelmanager - #3767
Merged
valentinewallace merged 1 commit into
lightningdevkit:mainfrom 
joostjager:fmt-chanmgr
Jun 4, 2025
Merged
Run rustfmt on channelmanager#3767
valentinewallace merged 1 commit into
lightningdevkit:mainfrom 
joostjager:fmt-chanmgr
Conversation
@joostjager
@joostjagerjoostjager
commented
May 5, 2025•

edited

Loading

Copy link


Copy Markdown

Contributor
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
ldk-reviews-bot
commented
May 5, 2025•

edited

Loading

Copy link


Copy Markdown

👋 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
ldk-reviews-bot
commented
May 7, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 10, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 12, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 14, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 17, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 19, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 21, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 24, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 26, 2025

Copy link


Copy Markdown

🔔 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
joostjager
commented
May 27, 2025

Copy link


Copy Markdown

ContributorAuthor
Pushed a new take on channelmanager formatting. Used an automated script to insert the skip directives before every function.
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
2 times, most recently
from
f033d19 to
d7b433eCompareMay 27, 2025 09:46
@codecov
codecovBot
commented
May 27, 2025•

edited

Loading

Copy link


Copy Markdown

Codecov Report
All modified and coverable lines are covered by tests ✅
Project coverage is 89.90%. Comparing base (901f72d) to head (3c9a1b5).

Report is 2 commits behind head on main.
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.

📢 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
ldk-reviews-bot
commented
May 28, 2025

Copy link


Copy Markdown

🔔 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
valentinewallace
reviewed
May 28, 2025
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
6 times, most recently
from
fd5301e to
ae7d478CompareMay 28, 2025 10:29
@joostjager
joostjager
commented
May 28, 2025

Copy link


Copy Markdown

ContributorAuthor
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
joostjager
commented
May 28, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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
jkczyz
commented
May 28, 2025

Copy link


Copy Markdown

Contributor
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
valentinewallace
previously approved these changes
May 29, 2025
View reviewed changes
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
3 times, most recently
from
08ccc6f to
edf9c57CompareMay 30, 2025 08:20
@joostjager
joostjager
commented
May 30, 2025

Copy link


Copy Markdown

ContributorAuthor
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
ldk-reviews-bot
commented
May 31, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
Jun 2, 2025

Copy link


Copy Markdown

🔔 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
valentinewallace
commented
Jun 2, 2025

Copy link


Copy Markdown

Contributor
Fixup LGTM
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
2 times, most recently
from
384ca93 to
83f41ddCompareJune 3, 2025 11:56
@joostjager
joostjager
commented
Jun 3, 2025

Copy link


Copy Markdown

ContributorAuthor
Rebased. Would be nice to move towards a conclusion on this approach.
@ldk-reviews-bot
ldk-reviews-bot
commented
Jun 4, 2025

Copy link


Copy Markdown

🔔 13th 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.
tnull
tnull
reviewed
Jun 4, 2025
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems this needs yet another rebase unfortunately.
@joostjager
joostjager
commented
Jun 4, 2025

Copy link


Copy Markdown

ContributorAuthor
Rebased
valentinewallace
valentinewallace
previously approved these changes
Jun 4, 2025
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@joostjager
Add rustfmt skip directives to channelmanager.rs
28116be
This allows devs to enable rustfmt on a per-function level.
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
3 times, most recently
from
3c9a1b5 to
28116beCompareJune 4, 2025 15:30
@valentinewallace
valentinewallace
commented
Jun 4, 2025

Copy link


Copy Markdown

Contributor
I heard from @joostjager that @TheBlueMatt is okay with landing this. Seems legit! Landing!
@valentinewallace
valentinewallace
merged commit 7b62ea4
into
lightningdevkit:mainJun 4, 2025

This was referenced Jun 4, 2025
 Merged

 Merged

@valentinewallacevalentinewallace
mentioned this pull request
Jun 12, 2025
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@tnulltnulltnull left review comments
@valentinewallacevalentinewallacevalentinewallace approved these changes
@TheBlueMattTheBlueMattAwaiting requested review from TheBlueMatt

Assignees


No one assigned


Labels

rustfmt

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@joostjager@ldk-reviews-bot@jkczyz@valentinewallace@tnull

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Run rustfmt on `channelmanager` by joostjager · Pull Request #3767 · lightningdevkit/rust-lightning · GitHub
Skip to content
Run rustfmt on channelmanager - #3767
Merged
valentinewallace merged 1 commit into
lightningdevkit:mainfrom 
joostjager:fmt-chanmgr
Jun 4, 2025
Merged
Run rustfmt on channelmanager#3767
valentinewallace merged 1 commit into
lightningdevkit:mainfrom 
joostjager:fmt-chanmgr
Conversation
@joostjager
@joostjagerjoostjager
commented
May 5, 2025•

edited

Loading

Copy link


Copy Markdown

Contributor
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
ldk-reviews-bot
commented
May 5, 2025•

edited

Loading

Copy link


Copy Markdown

👋 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
ldk-reviews-bot
commented
May 7, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 10, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 12, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 14, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 17, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 19, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 21, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 24, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 26, 2025

Copy link


Copy Markdown

🔔 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
joostjager
commented
May 27, 2025

Copy link


Copy Markdown

ContributorAuthor
Pushed a new take on channelmanager formatting. Used an automated script to insert the skip directives before every function.
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
2 times, most recently
from
f033d19 to
d7b433eCompareMay 27, 2025 09:46
@codecov
codecovBot
commented
May 27, 2025•

edited

Loading

Copy link


Copy Markdown

Codecov Report
All modified and coverable lines are covered by tests ✅
Project coverage is 89.90%. Comparing base (901f72d) to head (3c9a1b5).

Report is 2 commits behind head on main.
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.

📢 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
ldk-reviews-bot
commented
May 28, 2025

Copy link


Copy Markdown

🔔 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
valentinewallace
reviewed
May 28, 2025
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
6 times, most recently
from
fd5301e to
ae7d478CompareMay 28, 2025 10:29
@joostjager
joostjager
commented
May 28, 2025

Copy link


Copy Markdown

ContributorAuthor
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
joostjager
commented
May 28, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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
jkczyz
commented
May 28, 2025

Copy link


Copy Markdown

Contributor
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
valentinewallace
previously approved these changes
May 29, 2025
View reviewed changes
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
3 times, most recently
from
08ccc6f to
edf9c57CompareMay 30, 2025 08:20
@joostjager
joostjager
commented
May 30, 2025

Copy link


Copy Markdown

ContributorAuthor
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
ldk-reviews-bot
commented
May 31, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
Jun 2, 2025

Copy link


Copy Markdown

🔔 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
valentinewallace
commented
Jun 2, 2025

Copy link


Copy Markdown

Contributor
Fixup LGTM
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
2 times, most recently
from
384ca93 to
83f41ddCompareJune 3, 2025 11:56
@joostjager
joostjager
commented
Jun 3, 2025

Copy link


Copy Markdown

ContributorAuthor
Rebased. Would be nice to move towards a conclusion on this approach.
@ldk-reviews-bot
ldk-reviews-bot
commented
Jun 4, 2025

Copy link


Copy Markdown

🔔 13th 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.
tnull
tnull
reviewed
Jun 4, 2025
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems this needs yet another rebase unfortunately.
@joostjager
joostjager
commented
Jun 4, 2025

Copy link


Copy Markdown

ContributorAuthor
Rebased
valentinewallace
valentinewallace
previously approved these changes
Jun 4, 2025
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@joostjager
Add rustfmt skip directives to channelmanager.rs
28116be
This allows devs to enable rustfmt on a per-function level.
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
3 times, most recently
from
3c9a1b5 to
28116beCompareJune 4, 2025 15:30
@valentinewallace
valentinewallace
commented
Jun 4, 2025

Copy link


Copy Markdown

Contributor
I heard from @joostjager that @TheBlueMatt is okay with landing this. Seems legit! Landing!
@valentinewallace
valentinewallace
merged commit 7b62ea4
into
lightningdevkit:mainJun 4, 2025

This was referenced Jun 4, 2025
 Merged

 Merged

@valentinewallacevalentinewallace
mentioned this pull request
Jun 12, 2025
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@tnulltnulltnull left review comments
@valentinewallacevalentinewallacevalentinewallace approved these changes
@TheBlueMattTheBlueMattAwaiting requested review from TheBlueMatt

Assignees


No one assigned


Labels

rustfmt

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@joostjager@ldk-reviews-bot@jkczyz@valentinewallace@tnull

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Run rustfmt on `channelmanager` by joostjager · Pull Request #3767 · lightningdevkit/rust-lightning · GitHub
Skip to content
Run rustfmt on channelmanager - #3767
Merged
valentinewallace merged 1 commit into
lightningdevkit:mainfrom 
joostjager:fmt-chanmgr
Jun 4, 2025
Merged
Run rustfmt on channelmanager#3767
valentinewallace merged 1 commit into
lightningdevkit:mainfrom 
joostjager:fmt-chanmgr
Conversation
@joostjager
@joostjagerjoostjager
commented
May 5, 2025•

edited

Loading

Copy link


Copy Markdown

Contributor
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
ldk-reviews-bot
commented
May 5, 2025•

edited

Loading

Copy link


Copy Markdown

👋 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
ldk-reviews-bot
commented
May 7, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 10, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 12, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 14, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 17, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 19, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 21, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 24, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 26, 2025

Copy link


Copy Markdown

🔔 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
joostjager
commented
May 27, 2025

Copy link


Copy Markdown

ContributorAuthor
Pushed a new take on channelmanager formatting. Used an automated script to insert the skip directives before every function.
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
2 times, most recently
from
f033d19 to
d7b433eCompareMay 27, 2025 09:46
@codecov
codecovBot
commented
May 27, 2025•

edited

Loading

Copy link


Copy Markdown

Codecov Report
All modified and coverable lines are covered by tests ✅
Project coverage is 89.90%. Comparing base (901f72d) to head (3c9a1b5).

Report is 2 commits behind head on main.
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.

📢 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
ldk-reviews-bot
commented
May 28, 2025

Copy link


Copy Markdown

🔔 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
valentinewallace
reviewed
May 28, 2025
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
6 times, most recently
from
fd5301e to
ae7d478CompareMay 28, 2025 10:29
@joostjager
joostjager
commented
May 28, 2025

Copy link


Copy Markdown

ContributorAuthor
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
joostjager
commented
May 28, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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
jkczyz
commented
May 28, 2025

Copy link


Copy Markdown

Contributor
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
valentinewallace
previously approved these changes
May 29, 2025
View reviewed changes
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
3 times, most recently
from
08ccc6f to
edf9c57CompareMay 30, 2025 08:20
@joostjager
joostjager
commented
May 30, 2025

Copy link


Copy Markdown

ContributorAuthor
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
ldk-reviews-bot
commented
May 31, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
Jun 2, 2025

Copy link


Copy Markdown

🔔 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
valentinewallace
commented
Jun 2, 2025

Copy link


Copy Markdown

Contributor
Fixup LGTM
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
2 times, most recently
from
384ca93 to
83f41ddCompareJune 3, 2025 11:56
@joostjager
joostjager
commented
Jun 3, 2025

Copy link


Copy Markdown

ContributorAuthor
Rebased. Would be nice to move towards a conclusion on this approach.
@ldk-reviews-bot
ldk-reviews-bot
commented
Jun 4, 2025

Copy link


Copy Markdown

🔔 13th 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.
tnull
tnull
reviewed
Jun 4, 2025
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems this needs yet another rebase unfortunately.
@joostjager
joostjager
commented
Jun 4, 2025

Copy link


Copy Markdown

ContributorAuthor
Rebased
valentinewallace
valentinewallace
previously approved these changes
Jun 4, 2025
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@joostjager
Add rustfmt skip directives to channelmanager.rs
28116be
This allows devs to enable rustfmt on a per-function level.
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
3 times, most recently
from
3c9a1b5 to
28116beCompareJune 4, 2025 15:30
@valentinewallace
valentinewallace
commented
Jun 4, 2025

Copy link


Copy Markdown

Contributor
I heard from @joostjager that @TheBlueMatt is okay with landing this. Seems legit! Landing!
@valentinewallace
valentinewallace
merged commit 7b62ea4
into
lightningdevkit:mainJun 4, 2025

This was referenced Jun 4, 2025
 Merged

 Merged

@valentinewallacevalentinewallace
mentioned this pull request
Jun 12, 2025
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@tnulltnulltnull left review comments
@valentinewallacevalentinewallacevalentinewallace approved these changes
@TheBlueMattTheBlueMattAwaiting requested review from TheBlueMatt

Assignees


No one assigned


Labels

rustfmt

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@joostjager@ldk-reviews-bot@jkczyz@valentinewallace@tnull

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
Run rustfmt on `channelmanager` by joostjager · Pull Request #3767 · lightningdevkit/rust-lightning · GitHub
Skip to content
Run rustfmt on channelmanager - #3767
Merged
valentinewallace merged 1 commit into
lightningdevkit:mainfrom 
joostjager:fmt-chanmgr
Jun 4, 2025
Merged
Run rustfmt on channelmanager#3767
valentinewallace merged 1 commit into
lightningdevkit:mainfrom 
joostjager:fmt-chanmgr
Conversation
@joostjager
@joostjagerjoostjager
commented
May 5, 2025•

edited

Loading

Copy link


Copy Markdown

Contributor
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
ldk-reviews-bot
commented
May 5, 2025•

edited

Loading

Copy link


Copy Markdown

👋 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
ldk-reviews-bot
commented
May 7, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 10, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 12, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 14, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 17, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 19, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 21, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 24, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 26, 2025

Copy link


Copy Markdown

🔔 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
joostjager
commented
May 27, 2025

Copy link


Copy Markdown

ContributorAuthor
Pushed a new take on channelmanager formatting. Used an automated script to insert the skip directives before every function.
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
2 times, most recently
from
f033d19 to
d7b433eCompareMay 27, 2025 09:46
@codecov
codecovBot
commented
May 27, 2025•

edited

Loading

Copy link


Copy Markdown

Codecov Report
All modified and coverable lines are covered by tests ✅
Project coverage is 89.90%. Comparing base (901f72d) to head (3c9a1b5).

Report is 2 commits behind head on main.
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.

📢 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
ldk-reviews-bot
commented
May 28, 2025

Copy link


Copy Markdown

🔔 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
valentinewallace
reviewed
May 28, 2025
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
6 times, most recently
from
fd5301e to
ae7d478CompareMay 28, 2025 10:29
@joostjager
joostjager
commented
May 28, 2025

Copy link


Copy Markdown

ContributorAuthor
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
joostjager
commented
May 28, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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
jkczyz
commented
May 28, 2025

Copy link


Copy Markdown

Contributor
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
valentinewallace
previously approved these changes
May 29, 2025
View reviewed changes
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
3 times, most recently
from
08ccc6f to
edf9c57CompareMay 30, 2025 08:20
@joostjager
joostjager
commented
May 30, 2025

Copy link


Copy Markdown

ContributorAuthor
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
ldk-reviews-bot
commented
May 31, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
Jun 2, 2025

Copy link


Copy Markdown

🔔 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
valentinewallace
commented
Jun 2, 2025

Copy link


Copy Markdown

Contributor
Fixup LGTM
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
2 times, most recently
from
384ca93 to
83f41ddCompareJune 3, 2025 11:56
@joostjager
joostjager
commented
Jun 3, 2025

Copy link


Copy Markdown

ContributorAuthor
Rebased. Would be nice to move towards a conclusion on this approach.
@ldk-reviews-bot
ldk-reviews-bot
commented
Jun 4, 2025

Copy link


Copy Markdown

🔔 13th 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.
tnull
tnull
reviewed
Jun 4, 2025
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems this needs yet another rebase unfortunately.
@joostjager
joostjager
commented
Jun 4, 2025

Copy link


Copy Markdown

ContributorAuthor
Rebased
valentinewallace
valentinewallace
previously approved these changes
Jun 4, 2025
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@joostjager
Add rustfmt skip directives to channelmanager.rs
28116be
This allows devs to enable rustfmt on a per-function level.
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
3 times, most recently
from
3c9a1b5 to
28116beCompareJune 4, 2025 15:30
@valentinewallace
valentinewallace
commented
Jun 4, 2025

Copy link


Copy Markdown

Contributor
I heard from @joostjager that @TheBlueMatt is okay with landing this. Seems legit! Landing!
@valentinewallace
valentinewallace
merged commit 7b62ea4
into
lightningdevkit:mainJun 4, 2025

This was referenced Jun 4, 2025
 Merged

 Merged

@valentinewallacevalentinewallace
mentioned this pull request
Jun 12, 2025
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@tnulltnulltnull left review comments
@valentinewallacevalentinewallacevalentinewallace approved these changes
@TheBlueMattTheBlueMattAwaiting requested review from TheBlueMatt

Assignees


No one assigned


Labels

rustfmt

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@joostjager@ldk-reviews-bot@jkczyz@valentinewallace@tnull

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Run rustfmt on `channelmanager` by joostjager · Pull Request #3767 · lightningdevkit/rust-lightning · GitHub
Skip to content
Run rustfmt on channelmanager - #3767
Merged
valentinewallace merged 1 commit into
lightningdevkit:mainfrom 
joostjager:fmt-chanmgr
Jun 4, 2025
Merged
Run rustfmt on channelmanager#3767
valentinewallace merged 1 commit into
lightningdevkit:mainfrom 
joostjager:fmt-chanmgr
Conversation
@joostjager
@joostjagerjoostjager
commented
May 5, 2025•

edited

Loading

Copy link


Copy Markdown

Contributor
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
ldk-reviews-bot
commented
May 5, 2025•

edited

Loading

Copy link


Copy Markdown

👋 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
ldk-reviews-bot
commented
May 7, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 10, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 12, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 14, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 17, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 19, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 21, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 24, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 26, 2025

Copy link


Copy Markdown

🔔 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
joostjager
commented
May 27, 2025

Copy link


Copy Markdown

ContributorAuthor
Pushed a new take on channelmanager formatting. Used an automated script to insert the skip directives before every function.
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
2 times, most recently
from
f033d19 to
d7b433eCompareMay 27, 2025 09:46
@codecov
codecovBot
commented
May 27, 2025•

edited

Loading

Copy link


Copy Markdown

Codecov Report
All modified and coverable lines are covered by tests ✅
Project coverage is 89.90%. Comparing base (901f72d) to head (3c9a1b5).

Report is 2 commits behind head on main.
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.

📢 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
ldk-reviews-bot
commented
May 28, 2025

Copy link


Copy Markdown

🔔 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
valentinewallace
reviewed
May 28, 2025
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
6 times, most recently
from
fd5301e to
ae7d478CompareMay 28, 2025 10:29
@joostjager
joostjager
commented
May 28, 2025

Copy link


Copy Markdown

ContributorAuthor
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
joostjager
commented
May 28, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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
jkczyz
commented
May 28, 2025

Copy link


Copy Markdown

Contributor
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
valentinewallace
previously approved these changes
May 29, 2025
View reviewed changes
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
3 times, most recently
from
08ccc6f to
edf9c57CompareMay 30, 2025 08:20
@joostjager
joostjager
commented
May 30, 2025

Copy link


Copy Markdown

ContributorAuthor
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
ldk-reviews-bot
commented
May 31, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
Jun 2, 2025

Copy link


Copy Markdown

🔔 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
valentinewallace
commented
Jun 2, 2025

Copy link


Copy Markdown

Contributor
Fixup LGTM
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
2 times, most recently
from
384ca93 to
83f41ddCompareJune 3, 2025 11:56
@joostjager
joostjager
commented
Jun 3, 2025

Copy link


Copy Markdown

ContributorAuthor
Rebased. Would be nice to move towards a conclusion on this approach.
@ldk-reviews-bot
ldk-reviews-bot
commented
Jun 4, 2025

Copy link


Copy Markdown

🔔 13th 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.
tnull
tnull
reviewed
Jun 4, 2025
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems this needs yet another rebase unfortunately.
@joostjager
joostjager
commented
Jun 4, 2025

Copy link


Copy Markdown

ContributorAuthor
Rebased
valentinewallace
valentinewallace
previously approved these changes
Jun 4, 2025
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@joostjager
Add rustfmt skip directives to channelmanager.rs
28116be
This allows devs to enable rustfmt on a per-function level.
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
3 times, most recently
from
3c9a1b5 to
28116beCompareJune 4, 2025 15:30
@valentinewallace
valentinewallace
commented
Jun 4, 2025

Copy link


Copy Markdown

Contributor
I heard from @joostjager that @TheBlueMatt is okay with landing this. Seems legit! Landing!
@valentinewallace
valentinewallace
merged commit 7b62ea4
into
lightningdevkit:mainJun 4, 2025

This was referenced Jun 4, 2025
 Merged

 Merged

@valentinewallacevalentinewallace
mentioned this pull request
Jun 12, 2025
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@tnulltnulltnull left review comments
@valentinewallacevalentinewallacevalentinewallace approved these changes
@TheBlueMattTheBlueMattAwaiting requested review from TheBlueMatt

Assignees


No one assigned


Labels

rustfmt

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@joostjager@ldk-reviews-bot@jkczyz@valentinewallace@tnull

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Run rustfmt on `channelmanager` by joostjager · Pull Request #3767 · lightningdevkit/rust-lightning · GitHub
Skip to content
Run rustfmt on channelmanager - #3767
Merged
valentinewallace merged 1 commit into
lightningdevkit:mainfrom 
joostjager:fmt-chanmgr
Jun 4, 2025
Merged
Run rustfmt on channelmanager#3767
valentinewallace merged 1 commit into
lightningdevkit:mainfrom 
joostjager:fmt-chanmgr
Conversation
@joostjager
@joostjagerjoostjager
commented
May 5, 2025•

edited

Loading

Copy link


Copy Markdown

Contributor
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
ldk-reviews-bot
commented
May 5, 2025•

edited

Loading

Copy link


Copy Markdown

👋 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
ldk-reviews-bot
commented
May 7, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 10, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 12, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 14, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 17, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 19, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 21, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 24, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 26, 2025

Copy link


Copy Markdown

🔔 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
joostjager
commented
May 27, 2025

Copy link


Copy Markdown

ContributorAuthor
Pushed a new take on channelmanager formatting. Used an automated script to insert the skip directives before every function.
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
2 times, most recently
from
f033d19 to
d7b433eCompareMay 27, 2025 09:46
@codecov
codecovBot
commented
May 27, 2025•

edited

Loading

Copy link


Copy Markdown

Codecov Report
All modified and coverable lines are covered by tests ✅
Project coverage is 89.90%. Comparing base (901f72d) to head (3c9a1b5).

Report is 2 commits behind head on main.
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.

📢 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
ldk-reviews-bot
commented
May 28, 2025

Copy link


Copy Markdown

🔔 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
valentinewallace
reviewed
May 28, 2025
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
6 times, most recently
from
fd5301e to
ae7d478CompareMay 28, 2025 10:29
@joostjager
joostjager
commented
May 28, 2025

Copy link


Copy Markdown

ContributorAuthor
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
joostjager
commented
May 28, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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
jkczyz
commented
May 28, 2025

Copy link


Copy Markdown

Contributor
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
valentinewallace
previously approved these changes
May 29, 2025
View reviewed changes
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
3 times, most recently
from
08ccc6f to
edf9c57CompareMay 30, 2025 08:20
@joostjager
joostjager
commented
May 30, 2025

Copy link


Copy Markdown

ContributorAuthor
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
ldk-reviews-bot
commented
May 31, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
Jun 2, 2025

Copy link


Copy Markdown

🔔 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
valentinewallace
commented
Jun 2, 2025

Copy link


Copy Markdown

Contributor
Fixup LGTM
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
2 times, most recently
from
384ca93 to
83f41ddCompareJune 3, 2025 11:56
@joostjager
joostjager
commented
Jun 3, 2025

Copy link


Copy Markdown

ContributorAuthor
Rebased. Would be nice to move towards a conclusion on this approach.
@ldk-reviews-bot
ldk-reviews-bot
commented
Jun 4, 2025

Copy link


Copy Markdown

🔔 13th 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.
tnull
tnull
reviewed
Jun 4, 2025
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems this needs yet another rebase unfortunately.
@joostjager
joostjager
commented
Jun 4, 2025

Copy link


Copy Markdown

ContributorAuthor
Rebased
valentinewallace
valentinewallace
previously approved these changes
Jun 4, 2025
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@joostjager
Add rustfmt skip directives to channelmanager.rs
28116be
This allows devs to enable rustfmt on a per-function level.
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
3 times, most recently
from
3c9a1b5 to
28116beCompareJune 4, 2025 15:30
@valentinewallace
valentinewallace
commented
Jun 4, 2025

Copy link


Copy Markdown

Contributor
I heard from @joostjager that @TheBlueMatt is okay with landing this. Seems legit! Landing!
@valentinewallace
valentinewallace
merged commit 7b62ea4
into
lightningdevkit:mainJun 4, 2025

This was referenced Jun 4, 2025
 Merged

 Merged

@valentinewallacevalentinewallace
mentioned this pull request
Jun 12, 2025
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@tnulltnulltnull left review comments
@valentinewallacevalentinewallacevalentinewallace approved these changes
@TheBlueMattTheBlueMattAwaiting requested review from TheBlueMatt

Assignees


No one assigned


Labels

rustfmt

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@joostjager@ldk-reviews-bot@jkczyz@valentinewallace@tnull

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();
Run rustfmt on `channelmanager` by joostjager · Pull Request #3767 · lightningdevkit/rust-lightning · GitHub
Skip to content
Run rustfmt on channelmanager - #3767
Merged
valentinewallace merged 1 commit into
lightningdevkit:mainfrom 
joostjager:fmt-chanmgr
Jun 4, 2025
Merged
Run rustfmt on channelmanager#3767
valentinewallace merged 1 commit into
lightningdevkit:mainfrom 
joostjager:fmt-chanmgr
Conversation
@joostjager
@joostjagerjoostjager
commented
May 5, 2025•

edited

Loading

Copy link


Copy Markdown

Contributor
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
ldk-reviews-bot
commented
May 5, 2025•

edited

Loading

Copy link


Copy Markdown

👋 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
ldk-reviews-bot
commented
May 7, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 10, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 12, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 14, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 17, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 19, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 21, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 24, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
May 26, 2025

Copy link


Copy Markdown

🔔 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
joostjager
commented
May 27, 2025

Copy link


Copy Markdown

ContributorAuthor
Pushed a new take on channelmanager formatting. Used an automated script to insert the skip directives before every function.
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
2 times, most recently
from
f033d19 to
d7b433eCompareMay 27, 2025 09:46
@codecov
codecovBot
commented
May 27, 2025•

edited

Loading

Copy link


Copy Markdown

Codecov Report
All modified and coverable lines are covered by tests ✅
Project coverage is 89.90%. Comparing base (901f72d) to head (3c9a1b5).

Report is 2 commits behind head on main.
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.

📢 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
ldk-reviews-bot
commented
May 28, 2025

Copy link


Copy Markdown

🔔 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
valentinewallace
reviewed
May 28, 2025
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
6 times, most recently
from
fd5301e to
ae7d478CompareMay 28, 2025 10:29
@joostjager
joostjager
commented
May 28, 2025

Copy link


Copy Markdown

ContributorAuthor
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
joostjager
commented
May 28, 2025•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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
jkczyz
commented
May 28, 2025

Copy link


Copy Markdown

Contributor
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
valentinewallace
previously approved these changes
May 29, 2025
View reviewed changes
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
3 times, most recently
from
08ccc6f to
edf9c57CompareMay 30, 2025 08:20
@joostjager
joostjager
commented
May 30, 2025

Copy link


Copy Markdown

ContributorAuthor
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
ldk-reviews-bot
commented
May 31, 2025

Copy link


Copy Markdown

🔔 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
ldk-reviews-bot
commented
Jun 2, 2025

Copy link


Copy Markdown

🔔 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
valentinewallace
commented
Jun 2, 2025

Copy link


Copy Markdown

Contributor
Fixup LGTM
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
2 times, most recently
from
384ca93 to
83f41ddCompareJune 3, 2025 11:56
@joostjager
joostjager
commented
Jun 3, 2025

Copy link


Copy Markdown

ContributorAuthor
Rebased. Would be nice to move towards a conclusion on this approach.
@ldk-reviews-bot
ldk-reviews-bot
commented
Jun 4, 2025

Copy link


Copy Markdown

🔔 13th 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.
tnull
tnull
reviewed
Jun 4, 2025
View reviewed changes
@tnulltnull
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems this needs yet another rebase unfortunately.
@joostjager
joostjager
commented
Jun 4, 2025

Copy link


Copy Markdown

ContributorAuthor
Rebased
valentinewallace
valentinewallace
previously approved these changes
Jun 4, 2025
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

@joostjager
Add rustfmt skip directives to channelmanager.rs
28116be
This allows devs to enable rustfmt on a per-function level.
@joostjager
joostjagerforce-pushed
the
fmt-chanmgr
branch
3 times, most recently
from
3c9a1b5 to
28116beCompareJune 4, 2025 15:30
@valentinewallace
valentinewallace
commented
Jun 4, 2025

Copy link


Copy Markdown

Contributor
I heard from @joostjager that @TheBlueMatt is okay with landing this. Seems legit! Landing!
@valentinewallace
valentinewallace
merged commit 7b62ea4
into
lightningdevkit:mainJun 4, 2025

This was referenced Jun 4, 2025
 Merged

 Merged

@valentinewallacevalentinewallace
mentioned this pull request
Jun 12, 2025
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@tnulltnulltnull left review comments
@valentinewallacevalentinewallacevalentinewallace approved these changes
@TheBlueMattTheBlueMattAwaiting requested review from TheBlueMatt

Assignees


No one assigned


Labels

rustfmt

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@joostjager@ldk-reviews-bot@jkczyz@valentinewallace@tnull