Skip to content
This repository has been archived by the owner on Aug 16, 2021. It is now read-only.

Added chain_err method to Error implementations #141

Merged
merged 18 commits into from Apr 25, 2017
Merged

Added chain_err method to Error implementations #141

merged 18 commits into from Apr 25, 2017

Conversation

TedDriggs
Copy link
Contributor

@TedDriggs TedDriggs commented Apr 7, 2017 •
edited

Add a new method Error::caused(self, error: F) -> Error where F : FnOnce() -> EK, EK : Into<ErrorKind> to the generated error type. This enables chaining on error instances exposed by methods such as Result::map_err without inventing a new Result wrapper. This aims to address #40

Open Questions

  1. Should this method be named chain_err to align with ResultExt?
  2. Does this change avoid the inference issues from Caused error #62? All tests pass, but I'm not sure if the tests covered that case.

Yamakaky
Yamakaky reviewed Apr 7, 2017
View reviewed changes
CHANGELOG.md Outdated
@@ -1,5 +1,7 @@
# Unreleased

- Add a new method for `Error`: `caused`. _PR to be added_
Copy link
Contributor

Choose a reason for hiding this comment

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

You can now add the PR number ^^

src/error_chain.rs Outdated
pub fn caused<F, EK>(self, error: F) -> $error_name
where F: FnOnce() -> EK, EK: Into<$error_kind_name> {
$error_name::with_chain(self, Self::from_kind(error().into()))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you consider adding this to the ChainedErr trait?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had not; it looks like it should be straightforward enough to include. I'll put that in now.

@Yamakaky
Copy link
Contributor

Sorry for the delay. Could you add some doc in lib.rs?

@TedDriggs
Copy link
Contributor Author

TedDriggs commented Apr 11, 2017 •
edited

Yes. Is caused the name you'd like me to use, or should I go with chain_err or something instead? Update: I went with chain_err, as it was easier to document when it was aligned with the method exposed on ResultExt, and it serves a conceptually similar purpose.

kgv
kgv reviewed Apr 15, 2017
View reviewed changes
CHANGELOG.md Outdated
@@ -1,5 +1,7 @@
# Unreleased

- [Add a new method for `Error`: `caused`.](https://github.com/brson/error-chain/pull/141)
Copy link

Choose a reason for hiding this comment

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

Need to change description caused -> chain_err

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch; I've fixed that and pushed a new update.

@TedDriggs TedDriggs changed the title Added caused method to Error implementations Added chain_err method to Error implementations Apr 17, 2017
@TedDriggs
Copy link
Contributor Author

@kgv / @Yamakaky, please let me know if you need anything else for this to move forward.

@Yamakaky
Copy link
Contributor

Sorry, I had a lot of things to do these times. SGTM!

@Yamakaky Yamakaky merged commit e781ed5 into rust-lang-deprecated:master Apr 25, 2017
@TedDriggs TedDriggs mentioned this pull request May 24, 2017
Closed
@TedDriggs
Copy link
Contributor Author

@Yamakaky thanks for the merge; do you mind publishing a new release with this and the other changes sitting in master?

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

@kgv kgv kgv left review comments

@Yamakaky Yamakaky Yamakaky left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@TedDriggs @Yamakaky @kgv