Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove error chain #44

Closed
wants to merge 2 commits into from

Conversation

adeschamps
Copy link
Contributor

This turned out to be really easy to do, so merge it or close as you prefer. Since it came in #43 that the cc crate no longer compiles on 1.28, I removed the error-chain dependency, which was transitively including cc. All the errors produced by this crate are just strings anyway. I replaced it with a similar Error and ErrorKind pair, and implemented std::error::Error.

I think this is unlikely to break anyone's code, but it's technically a breaking change.

@sebest
Copy link
Contributor

sebest commented Jan 27, 2021

any chance to merge this change?

error_chains seems to be not really maintain anymore (beyond maintenance), one issue that I'm facing is that error_chains is not 'Sync'.

See the following threads for more context:

Moving to std::error::Error (or thiserror) would solve this issue.

Thank you!

@zslayton
Copy link
Owner

This is a good change; thanks for putting it together and sorry it fell off my radar. If you could rebase it against master I'd be happy to merge it.

@sebest
Copy link
Contributor

sebest commented Jan 28, 2021

@adeschamps : can you rebase/merge this pull request?

Otherwise should we close this pull request and create a new one?

@zslayton
Copy link
Owner

Otherwise should we close this pull request and create a new one?

Instead of rebasing you can merge from master and push another commit to this PR. I'm going to squash/merge it anyway, so the end result will be the same.

@sebest
Copy link
Contributor

sebest commented Jan 28, 2021

@zslayton : can I push a commit to this PR even if I'm not the initial author?

@zslayton
Copy link
Owner

@zslayton : can I push a commit to this PR even if I'm not the initial author?

I don't believe the permissions will allow you to. I think GitHub would expect you to fork the PR branch, make some changes, and then open another PR to merge your changes back into @adeschamps' original branch.

If you'd like to clone the PR branch and open a new PR, we can work on getting yours merged instead.

@sebest sebest mentioned this pull request Jan 29, 2021
@sebest
Copy link
Contributor

sebest commented Jan 29, 2021

@zslayton I created a new PR that rebase this one on master: #65

@zslayton
Copy link
Owner

Closed in favor of #65, which contains the same changes. Thanks @sebest, @adeschamps!

@zslayton zslayton closed this Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants