Closed
Conversation
As per Rust's convention, this allows the removal of a build warning.
While `Error::description()` usage is deprecated, it allows us to define static string slices for the various error messages. Until more advance formatting is needed, it is better to use `description` instead of the `Display` trait as recommended by Rust. This also removes a build warning.
`ToString::to_string()` should be favored in place of the deprecated `Error::description()` trait function. However, using `to_string` adds a heap allocation due to its use of dynamic type `String`. It should be noted that std::io::Error provides a richer context in the `Display` trait implementation in the deprecated `description` one. By electing to use `description`, we consciously choose to stay in the stack over usability. This also removes a build warning.
Collaborator
|
This seems duplicative with #594. Reviews there would be appreciated :).
… On Apr 20, 2020, at 00:04, Franck Royer ***@***.***> wrote:
I have noticed few build warnings on master and I thought I'd give a go at suppressing them. So far I have not changed any software behaviour, just removed unused parentheses and removed the warning using an allow attribute.
I have justified the used of the allow attribute in the commit.
Do you prefer justifying the use of allow attribute with in code comments?
Also, I assume that we are keeping the used of description() in purpose. Let me know if you'd prefer to see an implementation and use of the Display trait instead (as per Rust's recommendation).
I have not removed the warning warning: field is never read: 'logger' for ChainWatchInterfaceUtil and KeysManager because indeed, it is never read and I cannot seem to grasp why this field is present.
Some guidance would be greatly appreciated 🙂
You can view, comment on, or merge this pull request online at:
#599
Commit Summary
Remove unused parentheses
Allow use of deprecated `Error::description` for `io::Error`
Allow use of deprecated `Error::description` for `DecodeError`
File Changes
M lightning/src/chain/keysinterface.rs (2)
M lightning/src/ln/msgs.rs (2)
Patch Links:
https://github.com/rust-bitcoin/rust-lightning/pull/599.patch
https://github.com/rust-bitcoin/rust-lightning/pull/599.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
Collaborator
|
#594 was merged, so gonna close this. As for the missing logger reads, those were added cause we figured we'd have something worth logging. We probably do, in fact, have things worth logging, we just never do. I think the right fix is to log something at least at the trace level. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
I have noticed few build warnings on master and I thought I'd give a go at suppressing them. So far I have not changed any software behaviour, just removed unused parentheses and removed the warning using an
allowattribute.I have justified the used of the
allowattribute in the commit.Do you prefer justifying the use of
allowattribute with in code comments?Also, I assume that we are keeping the used of
description()in purpose. Let me know if you'd prefer to see an implementation and use of theDisplaytrait instead (as per Rust's recommendation).I have not removed the warning
warning: field is never read: 'logger'forChainWatchInterfaceUtilandKeysManagerbecause indeed, it is never read and I cannot seem to grasp why this field is present.Some guidance would be greatly appreciated 🙂