Format the rust-bitcoin crate
#1434
Conversation
|
Oh and massive props to everyone patching the commit 4ea682edf1812d77cab3ab35f2e4256c4bfacd46
Author: Tobin C. Harding <me@tobin.cc>
Date: Fri Dec 2 14:52:46 2022 +1100
psbt: Enable the formatter
Remove the exclude for the `psbt` module. No formatting issues currently
exist in the `psbt` module - WIN!
diff --git a/rustfmt.toml b/rustfmt.toml
index a14aa3a7..1942f2c4 100644
--- a/rustfmt.toml
+++ b/rustfmt.toml
@@ -1,6 +1,5 @@
# Eventually this shoud be: ignore = []
ignore = [
- "bitcoin/src/psbt",
"hashes",
] |
|
Will need to be rebased on top of #1432 to pass CI |
There was a problem hiding this comment.
No, but in all seriousness, thank you, @tcharding. As someone who has spent quite a few hours reading this codebase, I think it could be very valuable for future contributors and consumers to see consistently formatted code.
bitcoin/src/amount.rs
Outdated
|
|
||
| use super::*; |
There was a problem hiding this comment.
This happens automatically? In the projects I've worked on we usually try to keep all imports together because it's hard to enforce any other convention.
There was a problem hiding this comment.
Definitely automatic, well I definitely don't remember moving it and I noticed it too. But that's why I request a second dev to do the changes with the tools - I have been known to make mistakes (cough psbt still needs doing).
| use super::{deserialize, serialize, Error, CheckedData, VarInt}; | ||
| use super::{Transaction, BlockHash, FilterHash, TxMerkleNode, TxOut, TxIn}; |
bitcoin/src/consensus/encode.rs
Outdated
| 0..=0xFC => 1, | ||
| 0xFD..=0xFFFF => 3, | ||
| 0x10000..=0xFFFFFFFF => 5, | ||
| _ => 9, |
There was a problem hiding this comment.
I suppose you could annotate this with #[rustfmt::skip] if you really like the original formatting.
There was a problem hiding this comment.
Does anyone prefer the original? I wasn't fussed either way.
|
|
||
| use crate::blockdata::transaction::Transaction; | ||
| use crate::consensus::{encode, Decodable, Encodable}; | ||
| use crate::error::Error::{self, BlockBadProofOfWork, BlockBadTarget}; | ||
| pub use crate::hash_types::BlockHash; |
There was a problem hiding this comment.
Perhaps for another time, but I like it when pub use statements appear in a separate import section. It makes it easier to see what is part of the public API of a module.
There was a problem hiding this comment.
Yes that is a definite pain point with the current config, I was unable to get rustfmt to behave as you suggest.
bitcoin/src/blockdata/block.rs
Outdated
| let prevhash = | ||
| Vec::from_hex("4ddccd549d28f385ab457e98d1b11ce80bfea2c5ab93015ade4973e400000000") | ||
| .unwrap(); |
There was a problem hiding this comment.
I wonder if this kind of code would format with less noise like this:
let prevhash = "4ddccd549d28f385ab457e98d1b11ce80bfea2c5ab93015ade4973e400000000";
let prevhash = Vec::from_hex(prevhash).unwrap();
There was a problem hiding this comment.
We want to migrate this to const literal implemented using the fact that str.len() is const fn. AFAIK putting it into variable would destroy it. I'm not sure what the length of that one is but hex!("...") might be short enough. If it's not I'd seriously consider changing max line length.
| pub const BITCOIN: Self = Self([111, 226, 140, 10, 182, 241, 179, 114, 193, 166, 162, 70, 174, 99, 247, 79, 147, 30, 131, 101, 225, 90, 8, 156, 104, 214, 25, 0, 0, 0, 0, 0]); | ||
| pub const BITCOIN: Self = Self([ | ||
| 111, 226, 140, 10, 182, 241, 179, 114, 193, 166, 162, 70, 174, 99, 247, 79, 147, 30, 131, | ||
| 101, 225, 90, 8, 156, 104, 214, 25, 0, 0, 0, 0, 0, | ||
| ]); |
There was a problem hiding this comment.
Could be nice to format these with 4 rows of 8 bytes and add the rustfmt::skip attribute. In a separate prior commit that is.
There was a problem hiding this comment.
I think we should be able to use hex!() once we bump MSRV to 1.48.0
| Self::IntegerOverflow(val) => write!(f, "{} seconds is too large to be encoded to a 16 bit 512 second interval", val), | ||
| Self::IncompatibleHeight(lock, height) => write!(f, "tried to satisfy lock {} with height: {}", lock, height), | ||
| Self::IncompatibleTime(lock, time) => write!(f, "tried to satisfy lock {} with time: {}", lock, time), | ||
| Self::IntegerOverflow(val) => write!( | ||
| f, | ||
| "{} seconds is too large to be encoded to a 16 bit 512 second interval", | ||
| val | ||
| ), | ||
| Self::IncompatibleHeight(lock, height) => | ||
| write!(f, "tried to satisfy lock {} with height: {}", lock, height), | ||
| Self::IncompatibleTime(lock, time) => | ||
| write!(f, "tried to satisfy lock {} with time: {}", lock, time), |
There was a problem hiding this comment.
The original was a bit nicer, but this is pretty boring code anyway.
There was a problem hiding this comment.
I agree, formatting of Error impls is often rubbish. Apart from using shorter strings I'm not sure how to fix this. There are many instances throughout the crate of this sort of horrible formatting.
I tried importing Self::* just now but the lines are still too long.
There was a problem hiding this comment.
This is better
use Error::*;
match *self {
IntegerOverflow(val) =>
write!(f, "{} seconds does not fit in a 16 bit 512 second interval", val),
IncompatibleHeight(lock, height) =>
write!(f, "tried to satisfy lock {} with height: {}", lock, height),
IncompatibleTime(lock, time) =>
write!(f, "tried to satisfy lock {} with time: {}", lock, time),
}I've added "audit code base for stupid formatting" to my todo list (that should cover your array suggestion above also)
| @@ -687,6 +692,7 @@ impl All { | |||
| (OP_VERIF, _) | (OP_VERNOTIF, _) | (OP_INVALIDOPCODE, _) => Class::IllegalOp, | |||
|
|
|||
| // 15 opcodes illegal in Legacy context | |||
| #[rustfmt::skip] | |||
There was a problem hiding this comment.
As in this probably belongs in a separate commit to 0c95bb7.
There was a problem hiding this comment.
Bollocks! You got me. However, review for the win. Will fix and re-spin ... and do psbt.
bitcoin/src/blockdata/script.rs
Outdated
| script = script.push_int(1); comp.push(81u8); assert_eq!(&script[..], &comp[..]); | ||
| script = script.push_int(0); comp.push(0u8); assert_eq!(&script[..], &comp[..]); | ||
| script = script.push_int(4); comp.push(84u8); assert_eq!(&script[..], &comp[..]); | ||
| script = script.push_int(-1); comp.push(79u8); assert_eq!(&script[..], &comp[..]); | ||
| script = script.push_int(1); | ||
| comp.push(81u8); | ||
| assert_eq!(&script[..], &comp[..]); | ||
| script = script.push_int(0); | ||
| comp.push(0u8); | ||
| assert_eq!(&script[..], &comp[..]); | ||
| script = script.push_int(4); | ||
| comp.push(84u8); | ||
| assert_eq!(&script[..], &comp[..]); | ||
| script = script.push_int(-1); | ||
| comp.push(79u8); | ||
| assert_eq!(&script[..], &comp[..]); |
There was a problem hiding this comment.
This could be another candidate for rustfmt::skip since the original does make it easier to understand the test.
There was a problem hiding this comment.
I'll go with you on this one, will fix in next iteration, cheers.
bitcoin/src/blockdata/script.rs
Outdated
| 10, | ||
| 100, | ||
| 255, | ||
| 256, | ||
| 1000, | ||
| 10000, | ||
| 25000, | ||
| 200000, | ||
| 5000000, | ||
| 1000000000, | ||
| (1 << 31) - 1, | ||
| -((1 << 31) - 1), |
|
@tcharding, Is this all the formatting to be done? I am getting a large diff with |
WTF they are all in |
|
Thanks for the reviews! Sorry for the mistakes, hopefully the range-diff will not be too much work to re-review when I force push. |
tcharding
force-pushed
the
12-02-format-rust-bitcoin
branch
from
4ea682e
to
e19e9f3
Compare
|
Seen to review suggestions. Please note while re-creating all the formatting patches after rebase I stumbled upon an issue with a code comment that gets moved incorrectly by the formatter. Fixed in a new patch |
|
Also, just so I don't create any more false hopes, please note the |
tcharding
force-pushed
the
12-02-format-rust-bitcoin
branch
from
e19e9f3
to
d12ea54
Compare
|
Rebased and redid |
|
Suggestion for you to not have to rebase this for eternity:
The last change before release (which means during the freeze period) will be formatting and activation of all files. I understand it may suck for you to read unformatted code but honestly, if I had to rebase unsized script or similarly large PR I'd be pissed off to the point of considering quitting. And rebasing this every time wastes your time and spams my notifications. |
I'm definitely not pushing this through because of that, I stare soley at this library all day so the code layout is "normal" for me however it is - I'm pushing this because I think it improves the experience and perception of our cade base for others. |
|
I'm not confident pushing the rustfmt skip patches without the formatting changes because I could miss new code. I can leave this until code freeze though, no worries. |
|
Sorry to use your review time up @luckysori and then switch this to draft :( |
tcharding
force-pushed
the
12-02-format-rust-bitcoin
branch
2 times, most recently
from
edb26b7
to
ef2da64
Compare
|
cc @luckysori in case you want to take a look. I had to redo some patches because I botched the rebase (specifically the |
tcharding
force-pushed
the
12-02-format-rust-bitcoin
branch
from
ef2da64
to
2325601
Compare
|
Rebase and redo formatting patches to resolve conflicts so they maintain "mechanical only no manual changes". |
tcharding
force-pushed
the
12-02-format-rust-bitcoin
branch
2 times, most recently
from
f6eb6ed
to
11d968a
Compare
|
Same again: Rebase and redo formatting patches to resolve conflicts so they maintain "mechanical only no manual changes". |
|
On ice until #1111 merges. |
|
Assuming I'm correct about #1714 being the only problem with this then it would be super cool to merge this as the first PR merged after release of 0.30 before everyone starts rebasing their old PRs. All the formatting changes are in patches titled for example |
|
Sounds good! Could use a rebase to fix CI. |
In preparation for running the formatter introduce a couple of local variables to reduce the line length and inhibit function call from being split over multiple lines. Refactor only, no logic changes.
We already import `super::*`, these other imports are useless.
Remove the exclude for the `consensus` module. Do not run the formatter, that will be done as a separate patch to aid review.
Run `cargo +nightly fmt`, no other manual changes.
In preparation for formatting the `crypto` module add a couple of `skip` attributes to keep arrays formatted 8 bytes per line.
Remove the exclude for the `crypto` module. Do not run the formatter, that will be done as a separate patch to aid review.
Run `cargo +nightly fmt`, no other manual changes.
Currently we have a code comment that is supposed to assist devs in maintaining the `network::constants::Network` type by failing to build if a new variant is added. This plays havoc with the formatter because the comment is hanging at the bottom of a match block and the formatting thinks its for the proceeding line of code. Instead of using a code comment add a panic so the unit test fails if a new variant is added to `network::constants::Network`.
Remove the exclude for the `psbt` module. Do not run the formatter, that will be done as a separate patch to aid review.
Run `cargo +nightly fmt`, no other manual changes.
Add `rustfmt::skip` attribute in a couple of places and then remove the exclude for the `blockdata` module. Do not run the formatter, that will be done as a separate patch to aid review.
Run `cargo +nightly fmt`, no other manual changes.
Remove the exclude for the `util` module. Do not run the formatter, that will be done as a separate patch to aid review.
Run `cargo +nightly fmt`, no other manual changes.
The `hashes` module contains a bunch of arrays, mostly formatted with 8 hex bytes on a line; add `rustfmt::skip` to keep the formatting of arrays as is. Remove the exclude for the `hashes` crate. Do not run the formatter, that will be done as a separate patch to aid review.
Run `cargo +nightly fmt`, no other manual changes.
tcharding
force-pushed
the
12-02-format-rust-bitcoin
branch
from
ac9b150
to
913575a
Compare
|
Rebased to pick up CI fix, no other changes. |
|
Reviewed every change by hand. Thanks for al the work you did working around it! It still fucks up some comment alignment but nothing egregiously wrong. Lol at that comment that you had to turn into a Overall this is definitely an improvement, and 95% of it is stuff I'm pretty confident won't change in the future. |
Legend, thanks man! |
Bit sad huh, but I"m not going to patch rustfmt so I'm not going to complain too much.
I agree its definitely an improvement - hopefully over time someone else will improve rustfmt for us, "summer of rustfmt" ;) |
| match network { | ||
| Network::Bitcoin => {}, | ||
| Network::Testnet => {}, | ||
| Network::Signet => {}, | ||
| Network::Regtest => {}, | ||
| // Update ChainHash::using_genesis_block and chain_hash_genesis_block with new variants. | ||
| _ => panic!("Update ChainHash::using_genesis_block and chain_hash_genesis_block with new variants"), |
There was a problem hiding this comment.
OMG, please no. Put the comment on top of match instead. Anyway, we need a macro for this so the comment won't be needed.
There was a problem hiding this comment.
I'm quite confident this panic will be compiled away even with no optimizations, and it's only in a test ... and I'm skeptical that a macro would be easier to read/maintain.
There was a problem hiding this comment.
The point is previously it was impossible to forget adding the branch now it's possible without any warning.
Single source of truth is super-important. Breaking it kicked my ass so hard so many times I don't want to ever do it again.
There was a problem hiding this comment.
Oh, I now realized what it actually does. I'm OK with this for now but still think macro is superior.
There was a problem hiding this comment.
Because this is test code, we will get a warning -- the test failing with a panic.
I'll take your "OK for now" as an ok to merge this -- we can fix it to use a macro after the fact.
There was a problem hiding this comment.
I intended to review this right after release but I guess it's fine. I will complain when I see something problematic later.
One final push crew, 16 patches, only a few are big.
All non-trivial formatting is done in separate patches so the changes can be verified mechanically.
With this applied the whole
rust-bitcoincrate will be formatted.Big thanks to everyone for putting up with the ongoing formatting PRs, no-one likes doing these but hopefully this an improvement to the project - especially in helping us get more contributors to the project.