Skip to content
This repository was archived by the owner on Sep 12, 2018. It is now read-only.

Use failure instead of error_chain#730

Closed
grigoryk wants to merge 0 commit intomasterfrom
grisha/failure
Closed

Use failure instead of error_chain#730
grigoryk wants to merge 0 commit intomasterfrom
grisha/failure

Conversation

@grigoryk
Copy link
Copy Markdown
Contributor

@grigoryk grigoryk commented Jun 1, 2018

This is an early WIP to get feedback.

So far the only non-mechanical bit was conversion of InvalidBinding, since it wraps a BindingError.

Eventually fixes #586.

@grigoryk grigoryk requested review from ncalexan and rnewman June 1, 2018 02:36
Copy link
Copy Markdown
Member

@ncalexan ncalexan left a comment

Choose a reason for hiding this comment

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

This looks basically like I'd expect. I think we should do a little duplication (declare Result<T>, duplicate bail!) to minimize the initial changes, and then we can decide if we want to drop bail!.

I'll look at doing the db crate, which needs a lot of love to declare non-string errors anyway.

ErrorKind,
Result,
};
use errors::AlgebrizerError;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not against declaring Result<T> = std::result::Result<T, errors::AlgebrizerError> to reduce the changes. Or even implementing bail! in each crate -- it's easy.

The Err(...)? pattern is odd to me -- I prefer the return Err(...) expression.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Using ? adds an implicit into to do conversion of errors, which is a little more compact than return Err(some_err.into()) or return some_result.map_err(|e| e.into()).

@grigoryk grigoryk force-pushed the grisha/failure branch 7 times, most recently from 44e1e55 to caac84e Compare June 5, 2018 20:49
@grigoryk grigoryk removed the request for review from rnewman June 6, 2018 01:24
@grigoryk
Copy link
Copy Markdown
Contributor Author

grigoryk commented Jun 7, 2018

This is bulk of the work. Needs another cleanup pass, and rebasing on top of master - query-parser is gone, algebrizer changed a bit, and #736 is about to land.

@grigoryk grigoryk force-pushed the grisha/failure branch 3 times, most recently from 3368a08 to 3e06a2b Compare June 20, 2018 19:46
@grigoryk grigoryk changed the title WIP converting from error_chain to failure Use failure instead of error_chain Jun 20, 2018
@grigoryk
Copy link
Copy Markdown
Contributor Author

Tests pass! I'm not super happy with how parts of this turned out, but it's not a good idea to let this fall behind the other work. We can iterate on the particulars once/if we hit problems along the line.

@grigoryk
Copy link
Copy Markdown
Contributor Author

@ncalexan do you mind giving this a lookover before I have to rebase it again? 😭

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use failure instead of error_chain

3 participants