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

main(): Fix deprecated causes() w/iter_causes() #106

Merged
merged 1 commit into from Aug 6, 2018

Conversation

userzimmermann
Copy link
Contributor

Fixes #104

Hi :) Since I'm completely new to Rust, it took me a while to figure this out... But that's the best way to learn! :D

Nevertheless I'm still a bit confused ;) causes() is defined in Fail. Just like iter_causes(). The first can be directy used as method on the Error trait (even though being deprecated), the second can't... o.O Why?

I also don't know if my solution really works, since I wasn't be able to raise a simple error somehow for testing :D How to do that?

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Thanks @userzimmermann!

This needs to be tweaked a little bit before we merge it, see inline comment below.

twiggy/twiggy.rs Outdated
@@ -19,7 +19,7 @@ fn main() {
let options = opt::Options::from_args();
if let Err(e) = run(&options) {
eprintln!("error: {}", e);
for c in e.causes().skip(1) {
for c in Fail::iter_causes(&e).skip(1) {
Copy link
Member

Choose a reason for hiding this comment

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

Two things:

  1. The iter_causes method already skips the first error in the chain, so we can remove the skip(1) bit now (yay!)
  2. Rather than use unified function calling syntax (UFCS) we should use method calling syntax: e.iter_causes()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @fitzgen :) Regarding 1: Oops! Forgot to re-push after reading that iter_causes() "...does the skip(1) dance for us..." ;)

@userzimmermann
Copy link
Contributor Author

userzimmermann commented Aug 6, 2018

Regarding 2 from #106 (comment)

Using method calling style e.iter_causes() strangely results in:

error[E0599]: no method named `iter_causes` found for type `traits::Error` in the current scope
  --> twiggy\./twiggy.rs:22:20
   |
22 |         for c in e.iter_causes() {
   |                    ^^^^^^^^^^^

This is that confusing thing I mentioned in my PR description, since there also doesn't seem to be any explicit traits::Error::causes() defined, only failure::Fail::causes(), but it can nevertheless be used via e.causes()

I already feel this PR will give me a huge initial boost in understanding Rust... ;P

@fitzgen
Copy link
Member

fitzgen commented Aug 6, 2018

Oh wow this is funky, it is a method on the trait object, not a provided trait method: https://docs.rs/failure/0.1.2/src/failure/lib.rs.html#227-229

I... don't know what the impetus for that was.

It should work if we do:

let e = &e as &Fail;
for c in e.iter_causes() {
    ...
}

If that doesn't do the trick, then we can just merge the current version of the PR!

@userzimmermann
Copy link
Contributor Author

userzimmermann commented Aug 6, 2018

I also just fought my way to the trait Fail and impl Fail definitions :D And definitely need to get more understanding about those concepts...

@userzimmermann
Copy link
Contributor Author

userzimmermann commented Aug 6, 2018

@fitzgen, is there no simpler way of casting than let e = &e as &Fail; ?

@userzimmermann
Copy link
Contributor Author

userzimmermann commented Aug 6, 2018

@fitzgen, your cast suggestion does the trick! :) And can be even simplified:

for c in (&e as &Fail).iter_causes() {
    ...
}

But IMO, Fail::iter_causes(&e) looks somehow nicer ;)

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Thanks for sticking with this @userzimmermann!

Sorry about the churn on the method calling syntax. I'm still confused why they opted to go for methods on trait objects rather than trait methods! I would have been tripped up by that too, when writing this the first time :-/

@fitzgen fitzgen merged commit 0ad52a5 into rustwasm:master Aug 6, 2018
@fitzgen
Copy link
Member

fitzgen commented Aug 6, 2018

But IMO, Fail::iter_causes(&e) looks somehow nicer ;)

agreed

@userzimmermann
Copy link
Contributor Author

Thanks @fitzgen for merging and the nice discussion! But my poll https://twitter.com/zimmermanncode/status/1026540101202059264 about which calling style to use wasn't over yet ;P Luckily everybody voted for Fail::iter_causes(&e) so far :D

I learned a lot from this PR! And will try to get even more involved in the Rust WebAssembly community...

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.

None yet

2 participants