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

Make Full and Empty generic over the error type #85

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

davidpdrsn
Copy link
Member

It is not uncommon to have a function that creatures a boxed body via Full or Empty:

fn box_body_from_bytes(bytes: Bytes) -> UnsyncBoxBody<Bytes, Error> {
    Full::new(bytes)
        .map_err(|e| match e {})
        .boxed_unsync()
}

The .map_err(|e| match e {}) dance is necessary because Full always uses Infallible as its error type, so we have to convert that into whatever error type we actually need. This will often be hyper's, tonic's, or axum's error type.

However if we make Full generic over the error type:

pub struct Full<D, E> { ... }

then Rust can just infer it and we avoid having to call map_err:

fn box_body_from_bytes(bytes: Bytes) -> UnsyncBoxBody<Bytes, Error> {
    Full::new(bytes).boxed_unsync()
}

I think this is quite a bit nicer. It is especially nice that we avoid having to type match e {} which is quite confusing unless you've see it before.

The downside to this is that if the error type cannot be infered you have to explicitly set it when creating a Full:

Full::<_, Error>::new(bytes)

That makes this a breaking change.

It is not uncommon to have a function that creatures a boxed body via
`Full` or `Empty`:

```rust
fn box_body_from_bytes(bytes: Bytes) -> UnsyncBoxBody<Bytes, Error> {
    Full::new(bytes)
        .map_err(|e| match e {})
        .boxed_unsync()
}
```

The `.map_err(|e| match e {})` dance is necessary because `Full` always
uses `Infallible` as its error type, so we have to convert that into
whatever error type we actually need. This will often be hyper's,
tonic's, or axum's error type.

However if we make `Full` generic over the error type:

```rust
pub struct Full<D, E> { ... }
```

then Rust can just infer it and we avoid having to call `map_err`:

```rust
fn box_body_from_bytes(bytes: Bytes) -> UnsyncBoxBody<Bytes, Error> {
    Full::new(bytes).boxed_unsync()
}
```

I think this is quite a bit nicer. It is especially nice that we avoid
having to type `match e {}` which is quite confusing unless you've see
it before.

The downside to this is that if the error type cannot be infered you
have to explicitly set it when creating a `Full`:

```rust
Full::<_, Error>::new(bytes)
```

That makes this a breaking change.
@davidpdrsn davidpdrsn changed the title Make Full's and Empty's error type generic Make Full and Empty generic over the error type Feb 8, 2023
fmt,
marker::PhantomData,
pin::Pin,
task::{Context, Poll},
};

/// A body that is always empty.
pub struct Empty<D> {
_marker: PhantomData<fn() -> D>,
pub struct Empty<D, E> {
Copy link
Member

Choose a reason for hiding this comment

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

Probably worth discussing: what if it still had a default? Empty<D, E = Infallible>? It seems useful, but there's probably some cons to consider too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that makes sense!

@davidpdrsn davidpdrsn requested a review from seanmonstar March 14, 2023 18:43
@acfoltzer
Copy link
Member

I'm in favor of this; it'd make testing of functions that otherwise accept "production" types much more straighforward.

With the addition of the default, would this be a breaking change?

@seanmonstar
Copy link
Member

I think the default keeps it from being breaking. But it also means simply Full::new(bytes) probably has to require the default, otherwise inference will break, unfortunately.

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