Skip to content

Conversation

@indietyp
Copy link
Contributor

@indietyp indietyp commented Jun 5, 2022

This PR introduces a new type, Either, as discussed in #59.

Either implements:

  • std::error::Error
  • bytes::Buf
  • http_body::Body
  • From<Result<L, R>>

Either also provides some methods like: flip(), map_left(), map_right(), map(), either(), as_ref(), into_inner().

Due to the limitations of pin-project-lite this PR does not use the pin_project! macro but instead uses a slightly adjusted expanded version.

This change is entirely backward compatible.

@davidpdrsn
Copy link
Member

This just my opinion but I don't think we need all the other impls, just Body. I think users are unlikely to need them. I've never needed them myself when using http-body.

Same goes for all the helper methods on Either. In my experience you just need an Either so you can return two different body types from your tower::Service. The body isn't consumed as an Either but as a generic B: Body so all the additional methods on Either aren't necessary.

@indietyp
Copy link
Contributor Author

indietyp commented Jun 6, 2022

I see. Currently, this implements Either for Body, Error, and Buf. I thought that maybe - at least for Error it'd be helpful to keep track of the error on both sides instead of restricting the error to a single type. I understand removing Either on Data - as it is unlikely that you work with two different data streams, but I am not so sure about Error, what do you think?

@davidpdrsn
Copy link
Member

I think we should use Box<dyn Error + Send + Sync> as the error type, similar to what tower does. You can read about the reasons why here.

@indietyp
Copy link
Contributor Author

indietyp commented Jun 6, 2022

Thanks for the hint. That makes a lot of sense. I have implemented your suggestions.

@LucioFranco
Copy link
Member

Cool, I think if you update my one nit then we can merge this. Thanks!

@indietyp
Copy link
Contributor Author

Awesome 🎉, I am probably going to be able to apply your suggestions by tomorrow.

@LucioFranco LucioFranco changed the title Either util util: Add Either util Jun 24, 2022
@LucioFranco LucioFranco merged commit 6d7dd17 into hyperium:master Jun 24, 2022
@indietyp indietyp mentioned this pull request Jun 24, 2022
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