Remove MSRV todo comments#952
Conversation
|
The "add source" one is a bit nontrivial and should be fixed alongside the rest of #820 but I think the rest are one-liners that you could just fix in this PR, now that 0.28 is out. |
|
Force push is total re-write. Requires #964 to pass CI. |
Kixunil
left a comment
There was a problem hiding this comment.
NACKing to prevent an accidental merge of a bug.
src/network/address.rs
Outdated
There was a problem hiding this comment.
This looks incorrect because swap_bytes swaps the bytes regardless of the platform while to_be_bytes should do nothing on big endian platforms. I suspect we're missing a test case here. Remember that this change is in the PR multiple times.
Note that the comment suggested a different approach which seems correct.
There was a problem hiding this comment.
Good catch!! Things were perfectly set up for this bug to go unnoticed.
Agreed we should have a unit test here (though I don't think we have any BE systems in our CI pipeline?)
There was a problem hiding this comment.
I believe s390x-unknown-linux-gnu is BE
There was a problem hiding this comment.
Good reviewing @Kixunil, thanks. Will add test and re-think.
There was a problem hiding this comment.
I'm pulling this patch out of the PR.
|
Changes in force push:
|
No need to manually create a vector and push each element, just use the `vec![]` macro.
Now that we are bumping the MSRV to greater than 1.30 we can use `trim_start_matches`. Use `trim_start_matches` and remove the clippy directive.
|
Changes in force push: Use |
Kixunil
left a comment
There was a problem hiding this comment.
ACK 99cc2ac9c6568d8bd289dd69c7ac3c902fa3757d
src/util/taproot.rs
Outdated
There was a problem hiding this comment.
Nit: since all other methods start at separate lines it feels natural for .collect(); to be on a separate line as well. IDK what rustfmt says about it, I just use that style. :)
There was a problem hiding this comment.
oh yes, you are correct. Thanks, will fix and force push.
Now that we are going to bump the MSRV above 1.31 we can use `chunks_exact`.
We no longer support Rust 1.29, we can use `contains` for ranges instead of doing so manually.
|
Changes in force push: Just the |
There was a problem hiding this comment.
ACK 831b026. Thanks for separating out the bytes commit. Makes it easy to merge this one.
| all::OP_CLTV => write!(f, "CLTV"), | ||
| all::OP_CSV => write!(f, "CSV"), | ||
| All {code: x} if x >= all::OP_NOP1.code && x <= all::OP_NOP10.code => write!(f, "NOP{}", x - all::OP_NOP1.code + 1), | ||
| All {code: x} if (all::OP_NOP1.code..=all::OP_NOP10.code).contains(&x) => write!(f, "NOP{}", x - all::OP_NOP1.code + 1), |
There was a problem hiding this comment.
I am still getting used to the new syntax here. But gotta learn sometime
There was a problem hiding this comment.
I didn't even like it at first but got convinced by argument that it prevents messing up of operators and such.
831b026 Use contains() instead of manual range (Tobin C. Harding) 6410095 Use chunks_exact (Tobin C. Harding) 3a0097b Use trim_start_matches (Tobin C. Harding) 0a19710 Use vec! macro instead of new followed by push (Tobin C. Harding) Pull request description: Now that 0.28 is out we do not need to support Rust 1.29 on `master`. Remove trivial MSRV `TODO`s from the code. (All these changes only rely on MSRV bumping to 1.31 so are easily within bounds.) ACKs for top commit: Kixunil: ACK 831b026 sanket1729: ACK 831b026 Tree-SHA512: f1ea594216ba7dfa24696b964ce296a8aea72dd2e16e11d3a43fe8b90c851abf59b1612b2b1311146e8070112f3834762584e4f0515b8f546f72af169eb4bda9
Now that 0.28 is out we do not need to support Rust 1.29 on
master.Remove trivial MSRV
TODOs from the code. (All these changes only rely on MSRV bumping to 1.31 so are easily within bounds.)