-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Improve subkey error reporting #4504
Conversation
NikVolf
commented
Dec 26, 2019
•
edited
Loading
edited
It looks like @NikVolf signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
bin/utils/subkey/src/main.rs
Outdated
Formatted(String), | ||
} | ||
|
||
fn execute_proc<C: Crypto>(matches: ArgMatches) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just use fn main() -> Result<(), Error>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not really pretty-print error this way, see nv-sk-err-msin
branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do this:
substrate/client/network/src/error.rs
Line 51 in c3a4c55
impl fmt::Debug for Error { |
The error message should look good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better, but still worse than here if you ask me (updated nv-sk-err-msin
branch)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only different I see is that there is no new line after Error:
in the output? I think hat is okay and less code is always better, IMHO^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bkchr updated
532eb15
to
aad663e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, ty!
One last nitpick :)
@@ -270,7 +270,24 @@ fn get_uri(match_name: &str, matches: &ArgMatches) -> String { | |||
} | |||
} | |||
|
|||
fn execute<C: Crypto>(matches: ArgMatches) | |||
#[derive(derive_more::Display, derive_more::From)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please change the function above to return an error as well? (get_uri)