Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Client API refactoring - limiting errors to crate-level error types #1525

Merged
merged 9 commits into from
Jul 5, 2016

Conversation

NikVolf
Copy link
Contributor

@NikVolf NikVolf commented Jul 1, 2016

No description provided.

@NikVolf NikVolf added the A0-pleasereview 🤓 Pull request needs code review. label Jul 1, 2016
@NikVolf NikVolf mentioned this pull request Jul 1, 2016
6 tasks
@gavofyork
Copy link
Contributor

this seems to have added code and faff with no obvious reasons for it.

@NikVolf
Copy link
Contributor Author

NikVolf commented Jul 3, 2016

@gavofyork
nope, just removed from one place, inserted in the place where errors can be serialized and added mapping

needed for rpc api

@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 5, 2016
@NikVolf NikVolf merged commit a2b4997 into master Jul 5, 2016

/// Api-level error for transaction import
#[derive(Debug, Clone, Binary)]
pub enum TransactionImportError {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this type necessary? Can't we just use TransactionError?

@NikVolf NikVolf deleted the client-api-refact2 branch July 5, 2016 08:16
fn from(e: Error) -> Self {
match e {
Error::Transaction(transaction_error) => TransactionImportError::Transaction(transaction_error),
_ => TransactionImportError::Other(format!("other block import error: {:?}", e)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

imo, we should never do this type of conversion, cause it makes TransactionImportError more generic then Error

@debris
Copy link
Collaborator

debris commented Jul 5, 2016

I see that this pr is already merged. I generally like the changes that it introduced, but it needs some polishing

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants