Linter fixes #27
Linter fixes #27
Conversation
libbitcoin/src/signer.rs
Outdated
| @@ -102,6 +102,7 @@ impl string_result_t { | |||
| pub fn success(data: impl ToString) -> string_result_t { | |||
| let (code, details) = match CString::new(data.to_string()) { | |||
| Ok(s) => (error_t::success, result_details_t { data: s.into_raw() }), | |||
| #[allow(clippy::needless_borrow)] | |||
There was a problem hiding this comment.
Why just not remove the borrow here?
There was a problem hiding this comment.
It's because the CString NulError is of a different type, and there was some case where Clippy couldn't make sense of things.
Works:
#[allow(clippy::needless_borrow)]
Err(err) => (error_t::invalid_result_data, (&err).into()),Does not work:
Err(err) => (error_t::invalid_result_data, err.into()),Why:
the trait bound `signer::result_details_t: std::convert::From<std::ffi::NulError>` is not satisfied
the following implementations were found:
<signer::result_details_t as std::convert::From<&E>>
required because of the requirements on the impl of `std::convert::Into<signer::result_details_t>` for `std::ffi::NulError`
However, doing the CString conversion manually works:
Err(err) => (error_t::invalid_result_data, result_details_t {
data: CString::new(err.to_string())
.expect("Null byte in string_result_t success code doc comments")
.into_raw(),
}),This was referencing the code in the from impl here:
https://github.com/LNP-BP/descriptor-wallet/pull/27/files#diff-d3399c793447433124719f22f76f514e89f4bff0280e4675af35ae071b376ee2L179
But that's less-than-ideal, since it's essentially identical code to the From implementation, duplicated over. It's not so simple to implement the error type for CStrings. Anyway, I've pushed that code, let me know if you have a better approach.
rust-toolchain.toml
Outdated
| @@ -0,0 +1,2 @@ | |||
| [toolchain] | |||
| channel = "nightly" | |||
There was a problem hiding this comment.
hm, not sure we want to default to the nightly channel
There was a problem hiding this comment.
I just noticed, as I tried setting this back to stable, it caused the issue I mentioned I had fixed by reverting to the old code mentioend in the comment here:
#27 (comment)
I'm not sure we can get this to pass clippy lints on stable at this time without also making breaking changes to dependents.
There was a problem hiding this comment.
It seems that we can again move to stable with the latest 0.59 release
There was a problem hiding this comment.
It's true, how convenient :)
| @@ -15,6 +15,7 @@ | |||
| // Coding conventions | |||
| #![recursion_limit = "256"] | |||
| #![deny(dead_code, /* missing_docs, */ warnings)] | |||
| #![allow(clippy::init_numbered_fields)] | |||
There was a problem hiding this comment.
Probably this will go away after resolving LNP-BP/client_side_validation#54
There was a problem hiding this comment.
Can you pls check that we still need this with the latest 0.59 stable release?
There was a problem hiding this comment.
Removing this allow statement presents this warning/error on 1.59:
hd/src/indexes.rs
Outdated
| Normal { | ||
| /// Unhardened derivation 0-based index value | ||
| index: UnhardenedIndex, | ||
| }, |
There was a problem hiding this comment.
I do not entirely get why do we need this. Is it something suggested by the linter?
This is a breaking change which will stop downstream projects from compiling, and will require a major release. Are we really that much needing it?
There was a problem hiding this comment.
It was originally suggested by the linter, but if that was a breaking put it back. It's basically caused by the error I mentioned and documented here:
LNP-BP/client_side_validation#54
But also, it's weird, once I put it back, the error is gone, which may also invalidate that issue. Good catch, either way, thank you. I wouldn't want to introduce bad code, nor would I want to waste your time.
Codecov Report
@@ Coverage Diff @@
## master #27 +/- ##
========================================
- Coverage 19.2% 19.0% -0.2%
========================================
Files 34 34
Lines 2748 2746 -2
========================================
- Hits 528 523 -5
- Misses 2220 2223 +3
Continue to review full report at Codecov.
|
| @@ -15,6 +15,7 @@ | |||
| // Coding conventions | |||
| #![recursion_limit = "256"] | |||
| #![deny(dead_code, /* missing_docs, */ warnings)] | |||
| #![allow(clippy::init_numbered_fields)] | |||
There was a problem hiding this comment.
Can you pls check that we still need this with the latest 0.59 stable release?
rust-toolchain.toml
Outdated
| @@ -0,0 +1,2 @@ | |||
| [toolchain] | |||
| channel = "nightly" | |||
There was a problem hiding this comment.
It seems that we can again move to stable with the latest 0.59 release
|
I explicitly pegged the toolchain in the rust-toolchain.toml to 1.59 in case folks forget to rustup update. It passes linter on stable, but I had to leave in the clippy allow for init_numbered_fields. |
|
@dr-orlovsky I just noticed that some additional commits were pushed to the master branch. What's going on there? And should we go back to using nightly due to the try trait v2 feature? |
|
I merged previous work on taproot support since we've got We should not regress to nightly since nightly with |
One notable lint:
https://rust-lang.github.io/rust-clippy/master/index.html#init_numbered_fields
Which was why I made the AccountStep enum more consistent. I did that work as a separate commit because it's a bit more involved change.
There were a couple of other cases where I could've tried fixing the lint by adding named fields instead of a positional tuple struct to the enum member, which would fix the issue, but I became concerned it was indicative of an oversight in the strict encoding portion of the client side validation library. I've made an issue for that here:
LNP-BP/client_side_validation#54