Use rustfmt to hint at clean ups for the codebase #806
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Kixunil
reviewed
Jan 24, 2022
apoelstra
previously approved these changes
Jan 24, 2022
fa4da46 to
e6aa68b
Compare
|
I removed the patch that does logical operators and force pushed, that means commint hash of the last 4 patches has change, no other changes introduced, please re-ack @apoelstra. I'll have a play with the logical operators as a separate PR. Thanks for the reviews! |
apoelstra
previously approved these changes
Jan 25, 2022
apoelstra
added a commit
that referenced
this pull request
Jan 26, 2022
df7bb03 Simplify read_scriptbool (Tobin Harding) 4b6e866 Refactor is_provably_unspendable (Tobin Harding) e54a2d6 Put && operator at front of line (Tobin Harding) f5512c4 Refactor is_p2pkh (Tobin Harding) 373ea89 Simplify read_scriptbool (Tobin Harding) 654b277 Add passing unit tests for read_scriptbool (Tobin Harding) Pull request description: In an effort to make the code clearer and more explicit, do various refactorings around logical operators. Each done as a separate patch to ease review and limit scope of discussion. Based on review of #806 ACKs for top commit: Kixunil: ACK df7bb03 apoelstra: ACK df7bb03 Tree-SHA512: 06460979d492eb38cefc147397338b7fd95320c66ce8e8b4f8e2b454bb35721ce308413690a0618bd19d695df56175646d4d0c619388c0268f7fd35d5a7b6a3d
|
The change in 011618d to the file I also missed doing the |
apoelstra
previously approved these changes
Mar 10, 2022
|
Now requires rebase due to doc changes merged :( |
Remove trailing whitespace from all rust source files. Done with: find . -name *.rs | xargs perl -pli -e "s/\s*$//"
Do various whitespace refactorings, of note: - Use space around equals e.g., 'since = "blah"' - Put return/break/continue on separate line Whitespace only, no logic changes.
The last statement of a function does not need an explicit `return` statement.
Use statement contains unneeded braces, remove them.
We have a few instances of strange indentation: - Incorrect number of characters - Usage of neither "Block" style or "View" style (elect to use "Block")
Add a pair of braces to improve readability.
Our usage of `where` statements is not uniform, nor is it inline with the typical layout suggested by `rustfmt`. Make an effort to be more uniform with usage of `where` statements. However, explicitly do _not_ do every usage since sometimes our usage favours terseness (all on a single line).
The implementations of `consensus_encode` use an unnecessary number of lines. Favour more terse code with no loss of clarity.
As we do for logical operators; put the `+` operator at the start of the line to make it more obvious and assist devs reading the code.
This function uses neither "Block" nor "Visual" style (as defined by `rustfmt`). This is unusual, code that is regular is less jarring to read. We tent to use "Block" style for functions so elect to do that here.
Macro match arms can use any parenthesis-like character (it seems), however since we are delimiting a block of code elect to use braces.
Vector initialisation uses neither "Block" nor "Visual" stlye, this is irregular for no added benefit. Elect to use "Block" style (as defined by `rustfmt`).
In this library we specifically do not use rustfmt and tend to favour terse statements that do not use extra lines unnecessarily. In order to help new devs understand the style modify code that seems to use an unnecessary number of lines. None of these changes should reduce the readability of the code.
The compiler can infer this type, no need for an explicit type annotation.
|
Changes in force push:
|
13b451f to
a77907d
Compare
apoelstra
approved these changes
Mar 14, 2022
sanket1729
approved these changes
Mar 15, 2022
There was a problem hiding this comment.
ACK a77907d.
I ran cargo fmt before the first commit and at the tip of this PR and noted that the diff was non-logical changes only.
@tcharding , some things that might also be included like spaces in bip158.rs, but that can be done in a later PR.
I think we should merge this PR now that it has two ACKs because I can imagine this will conflict with multiple things and will be annoying to maintain. Plus, it is a non-logical change so can get it before RC too.
diff --git a/src/blockdata/script.rs b/src/blockdata/script.rs
index 3e4d4f9..6f9dbc0 100644
--- a/src/blockdata/script.rs
+++ b/src/blockdata/script.rs
@@ -1484,14 +1484,14 @@ mod test {
let slop_v_nonmin_alt: Result<Vec<Instruction>, Error> =
nonminimal_alt.instructions().collect();
- assert_eq!(v_zero.unwrap(), vec![Instruction::PushBytes(&[])]);
- assert_eq!(v_zeropush.unwrap(), vec![Instruction::PushBytes(&[0])]);
+ assert_eq!(v_zero.unwrap(), vec![Instruction::PushBytes(&[]),]);
+ assert_eq!(v_zeropush.unwrap(), vec![Instruction::PushBytes(&[0]),]);
assert_eq!(
v_min.clone().unwrap(),
vec![
Instruction::PushBytes(&[105]),
- Instruction::Op(opcodes::OP_NOP3)
+ Instruction::Op(opcodes::OP_NOP3),
]
);
@@ -1501,7 +1501,7 @@ mod test {
v_nonmin_alt.clone().unwrap(),
vec![
Instruction::PushBytes(&[105, 0]),
- Instruction::Op(opcodes::OP_NOP3)
+ Instruction::Op(opcodes::OP_NOP3),
]
);
diff --git a/src/blockdata/transaction.rs b/src/blockdata/transaction.rs
index 311fd08..b1fabfb 100644
--- a/src/blockdata/transaction.rs
+++ b/src/blockdata/transaction.rs
@@ -723,7 +723,7 @@ impl fmt::Display for NonStandardSigHashType {
#[cfg_attr(docsrs, doc(cfg(feature = "std")))]
impl error::Error for NonStandardSigHashType {}
-/// Legacy Hashtype of an input's signature
+/// Legacy Hashtype of an input's signature.
#[deprecated(since = "0.28.0", note = "Please use [`EcdsaSigHashType`] instead")]
pub type SigHashType = EcdsaSigHashType;
diff --git a/src/blockdata/witness.rs b/src/blockdata/witness.rs
index d159d37..645a33d 100644
--- a/src/blockdata/witness.rs
+++ b/src/blockdata/witness.rs
@@ -366,7 +366,6 @@ mod test {
expected_wit[0],
tx.input[0].witness.second_to_last().unwrap().to_hex()
);
-
let tx_bytes_back = serialize(&tx);
assert_eq!(tx_bytes_back, tx_bytes);
}
diff --git a/src/consensus/encode.rs b/src/consensus/encode.rs
index b97c0f1..caf01c4 100644
--- a/src/consensus/encode.rs
+++ b/src/consensus/encode.rs
@@ -734,7 +734,7 @@ impl<T: Encodable> Encodable for sync::Arc<T> {
// Tuples
macro_rules! tuple_encode {
- ($($x:ident),*) => {
+ ($($x:ident),*) => (
impl <$($x: Encodable),*> Encodable for ($($x),*) {
#[inline]
#[allow(non_snake_case)]
@@ -756,7 +756,7 @@ macro_rules! tuple_encode {
Ok(($({let $x = Decodable::consensus_decode(&mut d)?; $x }),*))
}
}
- };
+ );
}
tuple_encode!(T0, T1);
diff --git a/src/network/stream_reader.rs b/src/network/stream_reader.rs
index 1cf4a9e..cf6e64c 100644
--- a/src/network/stream_reader.rs
+++ b/src/network/stream_reader.rs
@@ -213,7 +213,7 @@ mod test {
let istream = TcpStream::connect(format!("127.0.0.1:{}", port)).unwrap();
let reader = BufReader::new(istream);
- (handle, reader)
+ return (handle, reader);
}
#[test]
diff --git a/src/util/address.rs b/src/util/address.rs
index b3c3e95..6eda874 100644
--- a/src/util/address.rs
+++ b/src/util/address.rs
@@ -98,10 +98,17 @@ impl fmt::Display for Error {
Error::InvalidWitnessVersion(v) => write!(f, "invalid witness script version: {}", v),
Error::UnparsableWitnessVersion(_) => write!(f, "incorrect format of a witness version byte"),
Error::MalformedWitnessVersion => f.write_str("bitcoin script opcode does not match any known witness version, the script is malformed"),
- Error::InvalidWitnessProgramLength(l) => write!(f, "the witness program must be between 2 and 40 bytes in length: length={}", l),
- Error::InvalidSegwitV0ProgramLength(l) => write!(f, "a v0 witness program must be either of length 20 or 32 bytes: length={}", l),
- Error::UncompressedPubkey => write!(f, "an uncompressed pubkey was used where it is not allowed"),
- Error::ExcessiveScriptSize => write!(f, "Script size exceed 520 bytes"),
+ Error::InvalidWitnessProgramLength(l) => write!(f,
+ "the witness program must be between 2 and 40 bytes in length: length={}", l,
+ ),
+ Error::InvalidSegwitV0ProgramLength(l) => write!(f,
+ "a v0 witness program must be either of length 20 or 32 bytes: length={}", l,
+ ),
+ Error::UncompressedPubkey => write!(f,
+ "an uncompressed pubkey was used where it is not allowed",
+ ),
+ Error::ExcessiveScriptSize => write!(f,
+ "Script size exceed 520 bytes")
}
}
}
diff --git a/src/util/bip158.rs b/src/util/bip158.rs
index 10ee4e9..f96d532 100644
--- a/src/util/bip158.rs
+++ b/src/util/bip158.rs
@@ -29,21 +29,21 @@
//! fn get_script_for_coin(coin: &OutPoint) -> Result<Script, BlockFilterError> {
//! // get utxo ...
//! }
-//!
+//!
//! // create a block filter for a block (server side)
//! let filter = BlockFilter::new_script_filter(&block, get_script_for_coin)?;
//!
//! // or create a filter from known raw data
//! let filter = BlockFilter::new(content);
-//!
+//!
//! // read and evaluate a filter
-//!
+//!
//! let query: Iterator<Item=Script> = // .. some scripts you care about
//! if filter.match_any(&block_hash, &mut query.map(|s| s.as_bytes())) {
//! // get this block
//! }
//! ```
-//!
+//!
use prelude::*;
diff --git a/src/util/key.rs b/src/util/key.rs
index 81d6412..706237e 100644
--- a/src/util/key.rs
+++ b/src/util/key.rs
@@ -187,7 +187,7 @@ impl PublicKey {
/// Deserialize a public key from a slice
pub fn from_slice(data: &[u8]) -> Result<PublicKey, Error> {
- let compressed = match data.len() {
+ let compressed: bool = match data.len() {
33 => true,
65 => false,
len => {
diff --git a/src/util/psbt/mod.rs b/src/util/psbt/mod.rs
index 0bae3db..4f8ebc5 100644
--- a/src/util/psbt/mod.rs
+++ b/src/util/psbt/mod.rs
@@ -902,7 +902,7 @@ mod tests {
assert_eq!(
tx.txid(),
Txid::from_hex("75c5c9665a570569ad77dd1279e6fd4628a093c4dcbf8d41532614044c14c115")
- .unwrap(),
+ .unwrap()
);
let mut unknown: BTreeMap<raw::Key, Vec<u8>> = BTreeMap::new();
diff --git a/src/util/taproot.rs b/src/util/taproot.rs
index 1bf3a7f..7546da9 100644
--- a/src/util/taproot.rs
+++ b/src/util/taproot.rs
@@ -987,13 +987,11 @@ impl fmt::Display for TaprootBuilderError {
"Attempted to create a tree with two nodes at depth 0. There must\
only be a exactly one node at depth 0",
),
- TaprootBuilderError::InvalidMerkleTreeDepth(d) => {
- write!(
- f,
- "Merkle Tree depth({}) must be less than {}",
- d, TAPROOT_CONTROL_MAX_NODE_COUNT
- )
- }
+ TaprootBuilderError::InvalidMerkleTreeDepth(d) => write!(
+ f,
+ "Merkle Tree depth({}) must be less than {}",
+ d, TAPROOT_CONTROL_MAX_NODE_COUNT
+ ),
TaprootBuilderError::InvalidInternalKey(e) => {
write!(f, "Invalid Internal XOnly key : {}", e)
}|
Thanks @sanket1729, there are definitely more things that could be done. I just did a subset that I thought was non-controversial and did not do too much code churn (e.g. adding commas to lines, I actually started doing them then dropped it :) |
|
I'm gonna merge this. Hopefully it does not break too many PRs. I think we have only one -- #796 -- that we need to get in before the RC, in any case, and it has 3 ACKs (but some outstanding concerns from Kixunil). |
This was referenced Mar 17, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
rustfmtis still under discussion, while researching the topic I came across a maintainer of another project that does not userustfmtwho mentioned that he manually implemented therusfmtsuggestions that he liked ever month or so. This seemed like a good idea so I did it. This was extremely painful but I believe I have put together a PR that is non-controversial with well separated patches.Totally non urgent.