Replace rustfmt_excluded_files with rustfmt_skip cfg attribute by joostjager · Pull Request #3802 · lightningdevkit/rust-lightning

joostjager · 2025-05-26T12:38:28Z

This change removes the custom rustfmt_excluded_files mechanism and instead uses an attribute at the top of each excluded file.

No functional changes are introduced. This simplifies formatting workflows by enabling standard cargo fmt and editor format-on-save features without custom scripts.

The older rustfmt cfg attribute is used because newer formatting directives only support code blocks, which would require more effort to apply consistently. This commit focuses on improving developer experience by streamlining formatting setup, while more granular or comprehensive formatting can be addressed later.

ldk-reviews-bot · 2025-05-26T12:38:30Z

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

valentinewallace

Thanks, this looks great and works in my text editor!

valentinewallace · 2025-05-27T02:27:36Z

 	check_closed_event!(nodes[1], 1, reason, [node_a_id], channel_value);
 }

+#[xtest(feature = "_externalize_tests")]


Are the update_fee_test and channel.rs changes intentional? It seems like they could use their own commit if so

I have really no idea how that slipped through. Reverted.

valentinewallace · 2025-05-27T18:09:25Z

Requested @tnull's review since @TheBlueMatt is off review this week, so hopefully we can land.

tnull

Sure, why not. One comment, otherwise LGTM.

This change removes the custom `rustfmt_excluded_files` mechanism and instead uses the an attribute at the top of each excluded file. No functional changes are introduced. This simplifies formatting workflows by enabling standard `cargo fmt` and editor format-on-save features without custom scripts. The older `rustfmt` cfg attribute is used because newer formatting directives only support code blocks, which would require more effort to apply consistently. This commit focuses on improving developer experience by streamlining formatting setup, while more granular or comprehensive formatting can be addressed later.

tnull

LGTM

Landing this as the changes since Val's ACK are minimal.

joostjager · 2025-05-26T12:38:28Z

This change removes the custom rustfmt_excluded_files mechanism and instead uses an attribute at the top of each excluded file.

No functional changes are introduced. This simplifies formatting workflows by enabling standard cargo fmt and editor format-on-save features without custom scripts.

The older rustfmt cfg attribute is used because newer formatting directives only support code blocks, which would require more effort to apply consistently. This commit focuses on improving developer experience by streamlining formatting setup, while more granular or comprehensive formatting can be addressed later.

ldk-reviews-bot · 2025-05-26T12:38:30Z

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

valentinewallace

Thanks, this looks great and works in my text editor!

valentinewallace · 2025-05-27T02:27:36Z

 	check_closed_event!(nodes[1], 1, reason, [node_a_id], channel_value);
 }

+#[xtest(feature = "_externalize_tests")]


Are the update_fee_test and channel.rs changes intentional? It seems like they could use their own commit if so

I have really no idea how that slipped through. Reverted.

valentinewallace · 2025-05-27T18:09:25Z

Requested @tnull's review since @TheBlueMatt is off review this week, so hopefully we can land.

tnull

Sure, why not. One comment, otherwise LGTM.

This change removes the custom `rustfmt_excluded_files` mechanism and instead uses the an attribute at the top of each excluded file. No functional changes are introduced. This simplifies formatting workflows by enabling standard `cargo fmt` and editor format-on-save features without custom scripts. The older `rustfmt` cfg attribute is used because newer formatting directives only support code blocks, which would require more effort to apply consistently. This commit focuses on improving developer experience by streamlining formatting setup, while more granular or comprehensive formatting can be addressed later.

tnull

LGTM

Landing this as the changes since Val's ACK are minimal.

joostjager · 2025-05-26T12:38:28Z

This change removes the custom rustfmt_excluded_files mechanism and instead uses an attribute at the top of each excluded file.

No functional changes are introduced. This simplifies formatting workflows by enabling standard cargo fmt and editor format-on-save features without custom scripts.

The older rustfmt cfg attribute is used because newer formatting directives only support code blocks, which would require more effort to apply consistently. This commit focuses on improving developer experience by streamlining formatting setup, while more granular or comprehensive formatting can be addressed later.

ldk-reviews-bot · 2025-05-26T12:38:30Z

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

valentinewallace

Thanks, this looks great and works in my text editor!

valentinewallace · 2025-05-27T02:27:36Z

 	check_closed_event!(nodes[1], 1, reason, [node_a_id], channel_value);
 }

+#[xtest(feature = "_externalize_tests")]


Are the update_fee_test and channel.rs changes intentional? It seems like they could use their own commit if so

I have really no idea how that slipped through. Reverted.

valentinewallace · 2025-05-27T18:09:25Z

Requested @tnull's review since @TheBlueMatt is off review this week, so hopefully we can land.

tnull

Sure, why not. One comment, otherwise LGTM.

This change removes the custom `rustfmt_excluded_files` mechanism and instead uses the an attribute at the top of each excluded file. No functional changes are introduced. This simplifies formatting workflows by enabling standard `cargo fmt` and editor format-on-save features without custom scripts. The older `rustfmt` cfg attribute is used because newer formatting directives only support code blocks, which would require more effort to apply consistently. This commit focuses on improving developer experience by streamlining formatting setup, while more granular or comprehensive formatting can be addressed later.

tnull

LGTM

Landing this as the changes since Val's ACK are minimal.

joostjager · 2025-05-26T12:38:28Z

This change removes the custom rustfmt_excluded_files mechanism and instead uses an attribute at the top of each excluded file.

No functional changes are introduced. This simplifies formatting workflows by enabling standard cargo fmt and editor format-on-save features without custom scripts.

The older rustfmt cfg attribute is used because newer formatting directives only support code blocks, which would require more effort to apply consistently. This commit focuses on improving developer experience by streamlining formatting setup, while more granular or comprehensive formatting can be addressed later.

ldk-reviews-bot · 2025-05-26T12:38:30Z

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

valentinewallace

Thanks, this looks great and works in my text editor!

valentinewallace · 2025-05-27T02:27:36Z

 	check_closed_event!(nodes[1], 1, reason, [node_a_id], channel_value);
 }

+#[xtest(feature = "_externalize_tests")]


Are the update_fee_test and channel.rs changes intentional? It seems like they could use their own commit if so

I have really no idea how that slipped through. Reverted.

valentinewallace · 2025-05-27T18:09:25Z

Requested @tnull's review since @TheBlueMatt is off review this week, so hopefully we can land.

tnull

Sure, why not. One comment, otherwise LGTM.

This change removes the custom `rustfmt_excluded_files` mechanism and instead uses the an attribute at the top of each excluded file. No functional changes are introduced. This simplifies formatting workflows by enabling standard `cargo fmt` and editor format-on-save features without custom scripts. The older `rustfmt` cfg attribute is used because newer formatting directives only support code blocks, which would require more effort to apply consistently. This commit focuses on improving developer experience by streamlining formatting setup, while more granular or comprehensive formatting can be addressed later.

tnull

LGTM

Landing this as the changes since Val's ACK are minimal.

joostjager · 2025-05-26T12:38:28Z

This change removes the custom rustfmt_excluded_files mechanism and instead uses an attribute at the top of each excluded file.

No functional changes are introduced. This simplifies formatting workflows by enabling standard cargo fmt and editor format-on-save features without custom scripts.

The older rustfmt cfg attribute is used because newer formatting directives only support code blocks, which would require more effort to apply consistently. This commit focuses on improving developer experience by streamlining formatting setup, while more granular or comprehensive formatting can be addressed later.

ldk-reviews-bot · 2025-05-26T12:38:30Z

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

valentinewallace

Thanks, this looks great and works in my text editor!

valentinewallace · 2025-05-27T02:27:36Z

 	check_closed_event!(nodes[1], 1, reason, [node_a_id], channel_value);
 }

+#[xtest(feature = "_externalize_tests")]


Are the update_fee_test and channel.rs changes intentional? It seems like they could use their own commit if so

I have really no idea how that slipped through. Reverted.

valentinewallace · 2025-05-27T18:09:25Z

Requested @tnull's review since @TheBlueMatt is off review this week, so hopefully we can land.

tnull

Sure, why not. One comment, otherwise LGTM.

This change removes the custom `rustfmt_excluded_files` mechanism and instead uses the an attribute at the top of each excluded file. No functional changes are introduced. This simplifies formatting workflows by enabling standard `cargo fmt` and editor format-on-save features without custom scripts. The older `rustfmt` cfg attribute is used because newer formatting directives only support code blocks, which would require more effort to apply consistently. This commit focuses on improving developer experience by streamlining formatting setup, while more granular or comprehensive formatting can be addressed later.

tnull

LGTM

Landing this as the changes since Val's ACK are minimal.

joostjager · 2025-05-26T12:38:28Z

This change removes the custom rustfmt_excluded_files mechanism and instead uses an attribute at the top of each excluded file.

No functional changes are introduced. This simplifies formatting workflows by enabling standard cargo fmt and editor format-on-save features without custom scripts.

The older rustfmt cfg attribute is used because newer formatting directives only support code blocks, which would require more effort to apply consistently. This commit focuses on improving developer experience by streamlining formatting setup, while more granular or comprehensive formatting can be addressed later.

ldk-reviews-bot · 2025-05-26T12:38:30Z

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

valentinewallace

Thanks, this looks great and works in my text editor!

valentinewallace · 2025-05-27T02:27:36Z

 	check_closed_event!(nodes[1], 1, reason, [node_a_id], channel_value);
 }

+#[xtest(feature = "_externalize_tests")]


Are the update_fee_test and channel.rs changes intentional? It seems like they could use their own commit if so

I have really no idea how that slipped through. Reverted.

valentinewallace · 2025-05-27T18:09:25Z

Requested @tnull's review since @TheBlueMatt is off review this week, so hopefully we can land.

tnull

Sure, why not. One comment, otherwise LGTM.

This change removes the custom `rustfmt_excluded_files` mechanism and instead uses the an attribute at the top of each excluded file. No functional changes are introduced. This simplifies formatting workflows by enabling standard `cargo fmt` and editor format-on-save features without custom scripts. The older `rustfmt` cfg attribute is used because newer formatting directives only support code blocks, which would require more effort to apply consistently. This commit focuses on improving developer experience by streamlining formatting setup, while more granular or comprehensive formatting can be addressed later.

tnull

LGTM

Landing this as the changes since Val's ACK are minimal.

joostjager · 2025-05-26T12:38:28Z

This change removes the custom rustfmt_excluded_files mechanism and instead uses an attribute at the top of each excluded file.

No functional changes are introduced. This simplifies formatting workflows by enabling standard cargo fmt and editor format-on-save features without custom scripts.

The older rustfmt cfg attribute is used because newer formatting directives only support code blocks, which would require more effort to apply consistently. This commit focuses on improving developer experience by streamlining formatting setup, while more granular or comprehensive formatting can be addressed later.

ldk-reviews-bot · 2025-05-26T12:38:30Z

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

valentinewallace

Thanks, this looks great and works in my text editor!

valentinewallace · 2025-05-27T02:27:36Z

 	check_closed_event!(nodes[1], 1, reason, [node_a_id], channel_value);
 }

+#[xtest(feature = "_externalize_tests")]


Are the update_fee_test and channel.rs changes intentional? It seems like they could use their own commit if so

I have really no idea how that slipped through. Reverted.

valentinewallace · 2025-05-27T18:09:25Z

Requested @tnull's review since @TheBlueMatt is off review this week, so hopefully we can land.

tnull

Sure, why not. One comment, otherwise LGTM.

This change removes the custom `rustfmt_excluded_files` mechanism and instead uses the an attribute at the top of each excluded file. No functional changes are introduced. This simplifies formatting workflows by enabling standard `cargo fmt` and editor format-on-save features without custom scripts. The older `rustfmt` cfg attribute is used because newer formatting directives only support code blocks, which would require more effort to apply consistently. This commit focuses on improving developer experience by streamlining formatting setup, while more granular or comprehensive formatting can be addressed later.

tnull

LGTM

Landing this as the changes since Val's ACK are minimal.

joostjager · 2025-05-26T12:38:28Z

This change removes the custom rustfmt_excluded_files mechanism and instead uses an attribute at the top of each excluded file.

No functional changes are introduced. This simplifies formatting workflows by enabling standard cargo fmt and editor format-on-save features without custom scripts.

The older rustfmt cfg attribute is used because newer formatting directives only support code blocks, which would require more effort to apply consistently. This commit focuses on improving developer experience by streamlining formatting setup, while more granular or comprehensive formatting can be addressed later.

ldk-reviews-bot · 2025-05-26T12:38:30Z

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

valentinewallace

Thanks, this looks great and works in my text editor!

valentinewallace · 2025-05-27T02:27:36Z

 	check_closed_event!(nodes[1], 1, reason, [node_a_id], channel_value);
 }

+#[xtest(feature = "_externalize_tests")]


Are the update_fee_test and channel.rs changes intentional? It seems like they could use their own commit if so

I have really no idea how that slipped through. Reverted.

valentinewallace · 2025-05-27T18:09:25Z

Requested @tnull's review since @TheBlueMatt is off review this week, so hopefully we can land.

tnull

Sure, why not. One comment, otherwise LGTM.

This change removes the custom `rustfmt_excluded_files` mechanism and instead uses the an attribute at the top of each excluded file. No functional changes are introduced. This simplifies formatting workflows by enabling standard `cargo fmt` and editor format-on-save features without custom scripts. The older `rustfmt` cfg attribute is used because newer formatting directives only support code blocks, which would require more effort to apply consistently. This commit focuses on improving developer experience by streamlining formatting setup, while more granular or comprehensive formatting can be addressed later.

tnull

LGTM

Landing this as the changes since Val's ACK are minimal.

joostjager added the rustfmt label May 26, 2025

joostjager force-pushed the rustfmt-skip branch from bf380da to 8978050Compare May 26, 2025 12:46

joostjager requested review from TheBlueMatt and valentinewallace May 26, 2025 14:16

joostjager marked this pull request as ready for review May 26, 2025 14:29

valentinewallace reviewed May 27, 2025
View reviewed changes

joostjager force-pushed the rustfmt-skip branch 3 times, most recently from 1176fc2 to e7af289Compare May 27, 2025 07:34

joostjager requested a review from valentinewallace May 27, 2025 07:37

joostjager force-pushed the rustfmt-skip branch from e7af289 to f1f6db5Compare May 27, 2025 08:01

valentinewallace previously approved these changes May 27, 2025
View reviewed changes

valentinewallace requested a review from tnull May 27, 2025 18:09

tnull reviewed May 28, 2025
View reviewed changes

Comment threadCONTRIBUTING.md Outdated

joostjager dismissed valentinewallace’s stale review via d257b8aMay 28, 2025 09:05

joostjager force-pushed the rustfmt-skip branch from f1f6db5 to d257b8aCompare May 28, 2025 09:05

joostjager requested a review from tnull May 28, 2025 09:05

tnull approved these changes May 28, 2025
View reviewed changes

tnull merged commit 5688166 into lightningdevkit:mainMay 28, 2025

joostjager mentioned this pull request May 28, 2025
Run rustfmt on channelmanager#3767
Merged