Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

CLI: Error message cleanup #8804

Merged
merged 28 commits into from
Mar 13, 2020
Merged

Conversation

t-nelson
Copy link
Contributor

@t-nelson t-nelson commented Mar 11, 2020

Problem

CLI produces error message that expose implementation details and are generally unfriendly to the user

Summary of Changes

  • Consistent return types throughout client interface
  • Stop abusing io::Error
  • Surface full error stack to callers rather than squashing to a string

TODO

  • Handle remaining io::Error abusers in RpcClient
  • Extend return type consistency to CLI Box<dyn error::Error> will do for now
  • Pretty error messages

Fixes #7722

@t-nelson t-nelson added work in progress This isn't quite right yet noCI Suppress CI on this Pull Request labels Mar 11, 2020
#[error("transport transaction error: {0}")]
TransactionError(#[from] TransactionError),
#[error("transport custom error: {0}")]
Custom(String),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might kill the String here in favor of https://crates.io/crates/anyhow if time permits. Would allow removal of its companion variant in ClientError

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tough call. anyhow is another escape hatch. The team tends to make use of those too often.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was waffling on that notion. Probably better to yank the dependency-based variants out of ClientError in favor of our own variants and some From impls

@t-nelson t-nelson changed the title Cli error cleanup CLI: Error message cleanup Mar 11, 2020
@t-nelson t-nelson force-pushed the cli-error-cleanup branch from c7e8dde to 2c21011 Compare March 12, 2020 22:09
@t-nelson t-nelson removed the noCI Suppress CI on this Pull Request label Mar 12, 2020
@codecov
Copy link

codecov bot commented Mar 12, 2020

Codecov Report

Merging #8804 into master will increase coverage by 0.1%.
The diff coverage is 20.5%.

@@           Coverage Diff            @@
##           master   #8804     +/-   ##
========================================
+ Coverage      80%   80.1%   +0.1%     
========================================
  Files         265     266      +1     
  Lines       57496   57401     -95     
========================================
+ Hits        46014   46019      +5     
+ Misses      11482   11382    -100

CriesofCarrots
CriesofCarrots previously approved these changes Mar 12, 2020
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm too late for most of the good nits, so just a couple consistency nits in the main()s
Thanks for all this; big improvement!

cli/src/main.rs Outdated Show resolved Hide resolved
keygen/src/keygen.rs Outdated Show resolved Hide resolved
@mergify mergify bot dismissed stale reviews from garious and CriesofCarrots March 13, 2020 02:07

Pull request has been modified.

@t-nelson
Copy link
Contributor Author

@CriesofCarrots @garious @jackcmay I believe I've addressed all of the previous comments now

@garious
Copy link
Contributor

garious commented Mar 13, 2020

:shipit: please. Want this in 1.0.6.

@garious garious added this to the v1.0.6 milestone Mar 13, 2020
@garious garious added the v1.0 label Mar 13, 2020
garious
garious previously approved these changes Mar 13, 2020
@t-nelson t-nelson added the automerge Merge this Pull Request automatically once CI passes label Mar 13, 2020
@t-nelson
Copy link
Contributor Author

🤖 take the wheel!

@solana-grimes solana-grimes removed the automerge Merge this Pull Request automatically once CI passes label Mar 13, 2020
@solana-grimes
Copy link
Contributor

💔 Unable to automerge due to CI failure

@mvines
Copy link
Contributor

mvines commented Mar 13, 2020

🤦

@garious
Copy link
Contributor

garious commented Mar 13, 2020

@t-nelson t-nelson added the automerge Merge this Pull Request automatically once CI passes label Mar 13, 2020
@mergify mergify bot dismissed garious’s stale review March 13, 2020 05:27

Pull request has been modified.

@t-nelson t-nelson removed the v1.0 label Mar 13, 2020
This was referenced Mar 13, 2020
@solana-grimes solana-grimes merged commit fbf2dd1 into solana-labs:master Mar 13, 2020
@t-nelson t-nelson deleted the cli-error-cleanup branch March 13, 2020 06:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ugly CLI errors, again
6 participants