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

Migrate crates to use Error: From<S::Error> #387

Closed
LucioFranco opened this issue Dec 11, 2019 · 7 comments
Closed

Migrate crates to use Error: From<S::Error> #387

LucioFranco opened this issue Dec 11, 2019 · 7 comments
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. I-needs-decision Issues in need of decision. P-medium Medium priority

Comments

@LucioFranco
Copy link
Member

Currently, we have a mix and match of crates that use From bounds over Into bounds. This is causing issues when wanting to compose the services together. I suggest we move them all to From since that is what Try is based on.

cc @jonhoo @seanmonstar @carllerche @hawkw

@seanmonstar
Copy link
Collaborator

From is much more convenient to use with ?. The reason I used Into is because that's what std::io::Error::new uses. It's technically possible that things can implement Into and not From (though not the reverse), but it should be very few things?

@hawkw
Copy link
Member

hawkw commented Mar 31, 2020

I think it might be worth revisiting the decision to go with Into rather than From as part of #431. Not being able to automatically convert errors with ? is a bit of a pain.

@LucioFranco
Copy link
Member Author

I do find bounds that use from much harder to read than using into which feels very intuitive to me. If its at the cost of using ? I think its fine.

@jonhoo
Copy link
Collaborator

jonhoo commented Mar 31, 2020

I'd like to get a better sense of the trade-off space here. If we use Into, where is it possible/not possible to use ?. If we use From, where is it possible/not possible to use ?. What do other libraries do?

@jonhoo jonhoo added C-feature-request Category: A feature request, i.e: not implemented / a PR. I-needs-decision Issues in need of decision. P-high High priority P-medium Medium priority and removed P-high High priority labels Mar 31, 2020
@jonhoo jonhoo added this to the v0.4 milestone Mar 31, 2020
@jonhoo
Copy link
Collaborator

jonhoo commented Apr 25, 2020

I also just noticed today that the Rust documentation recommends using Into in bounds: https://doc.rust-lang.org/std/convert/trait.From.html. I feel like there's something missing in the story here. In some sense, it feels like it's wrong for ? to rely on From :/

@jonhoo
Copy link
Collaborator

jonhoo commented Apr 25, 2020

See also my comment here: rust-lang/rust#31436 (comment)

@LucioFranco LucioFranco removed this from the v0.4 milestone Nov 27, 2020
@hawkw
Copy link
Member

hawkw commented Dec 23, 2020

I believe we've decided all error bounds should be Into.

@hawkw hawkw closed this as completed Dec 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. I-needs-decision Issues in need of decision. P-medium Medium priority
Projects
None yet
Development

No branches or pull requests

4 participants