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

patch-rs binding #160

Merged
merged 4 commits into from
Dec 14, 2018
Merged

patch-rs binding #160

merged 4 commits into from
Dec 14, 2018

Conversation

juchiast
Copy link
Contributor

I'm using git dependency for patch-rs, because v0.2 is not published yet.

@juchiast
Copy link
Contributor Author

@dariusc93

I tried that, but the compiler complains:

error[E0277]: `(dyn std::error::Error + std::marker::Send + 'static)` cannot be shared between threads safely
 --> src/bindings/app/patch.rs:8:38
  |
8 |             this.0.process().map_err(rlua::Error::external)
  |                                      ^^^^^^^^^^^^^^^^^^^^^ `(dyn std::error::Error + std::marker::Send + 'static)` cannot be shared between threads safely
  |
  = help: the trait `std::marker::Sync` is not implemented for `(dyn std::error::Error + std::marker::Send + 'static)`
  = note: required because of the requirements on the impl of `std::marker::Sync` for `std::ptr::Unique<(dyn std::error::Error + std::marker::Send + 'static)>`
  = note: required because it appears within the type `std::boxed::Box<(dyn std::error::Error + std::marker::Send + 'static)>`
  = note: required because it appears within the type `std::option::Option<std::boxed::Box<(dyn std::error::Error + std::marker::Send + 'static)>>`
  = note: required because it appears within the type `error_chain::State`
  = note: required because it appears within the type `patch_rs::PatchError`
  = note: required because of the requirements on the impl of `failure::Fail` for `patch_rs::PatchError`
  = note: required because of the requirements on the impl of `std::convert::From<patch_rs::PatchError>` for `failure::Error`
  = note: required because of the requirements on the impl of `std::convert::Into<failure::Error>` for `patch_rs::PatchError`
  = note: required by `rlua::Error::external`

How about this.0.process().ok()? The downside is that we lost the error message.

@juchiast
Copy link
Contributor Author

error-chain is not compatible with failure: rust-lang-deprecated/error-chain#240

I'm doing this:

            this.0
                .process()
                .map_err(|e| rlua::Error::external(failure::err_msg(e.to_string())))

dariusc93 added a commit to changeutils/patch-rs that referenced this pull request Dec 13, 2018
Due to error-chain not being maintain (from my understanding), it would be best to use a crate that would be more up to date and friendlier to use. This should also resolve an issue found in jazzdotdev/jazz#160 due to the way error-chain is designed.
@dariusc93
Copy link
Contributor

It looks like its related to how error-chain is designed. Honestly I havent used that myself especially since I thought error-chain crate was being deprecated in favor of the failure crate (since it wasnt being maintain to my knowledge). I went on and switch to the failure crate in the patch-rs. Could you check to make sure it is working for you as well and update it accordingly (including crate version, if deem necessary)?

@juchiast
Copy link
Contributor Author

@dariusc93
Ok, I tested current master branch of patch-rs, it works!
Publish a new version to crates.io then I'll update this PR.

@hedgar2017
Copy link
Contributor

@juchiast done, v0.3.0 is out there.
@dariusc93 I agree, I should have used failure from the beginning. Some of my projects still use error-chain so for some reason I went for that.

@dariusc93 dariusc93 merged commit e1af877 into jazzdotdev:master Dec 14, 2018
@naturallymitchell
Copy link
Member

naturallymitchell commented Dec 14, 2018

@hedgar2017 👍 👍
@juchiast 👍 👍
@dariusc93 👍 👍

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.

4 participants