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

Get rid of BoxError #57

Closed
MOZGIII opened this issue Dec 13, 2019 · 8 comments
Closed

Get rid of BoxError #57

MOZGIII opened this issue Dec 13, 2019 · 8 comments

Comments

@MOZGIII
Copy link

MOZGIII commented Dec 13, 2019

Why use BoxError in the HttpsConnector if an enum would work and won't be too complicated?

In my current code, I can't just use ? or .into on the error cause it's not Sized, and, although I can work around it pretty easily, I see no reason why we shouldn't use enum.

@seanmonstar
Copy link
Member

The reason for its use here is I didn't want to "wrap" the inner connectors errors, but instead just passing them on.

@MOZGIII
Copy link
Author

MOZGIII commented Dec 13, 2019

Well, yeah, I figured. Is there a reason why you don't want to wrap the errors?
I can send you a PR with the code with the wrapping if you'd like.

@seanmonstar
Copy link
Member

How come it can't be converted using ? or into?

@MOZGIII
Copy link
Author

MOZGIII commented Dec 13, 2019

image

@MOZGIII
Copy link
Author

MOZGIII commented Dec 14, 2019

I'm using failure::Error crate. The workaround is to .map_err(Error::from_boxed_compat) , but it's not pretty.

@MOZGIII MOZGIII mentioned this issue Dec 14, 2019
@MOZGIII
Copy link
Author

MOZGIII commented Dec 14, 2019

Submitted a PR.
I don't see a reason why one would want to not wrap errors in this case, so if you have one I'm really curious.

@seanmonstar
Copy link
Member

Mostly I didn't want to wrap if there was no additional context to add, since you'd need to look down the source chain one more time for err.downcast to match the wrapped error.

@MOZGIII
Copy link
Author

MOZGIII commented Dec 14, 2019

A mechanism like rust-lang/rust#58289 resolves that issue almost entirely, and to avoid doing a match over the subtypes one could just to err.source().downcast() - that is not much more verbose but does just pretty much the same as downcast on currently present BoxError (one couldn't check for ForceHttpsButUriNotHttps that way too anyway).

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 a pull request may close this issue.

2 participants