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

Implement Clone on With combinator #2290

Merged

Conversation

akhramov
Copy link
Contributor

Consider the following use-case of mpsc channels. There are two
categories of producers which produce items of two distinct types,
later unified in one using With combinator:

let (sender, receiver) = mspc::channel(100);

let download_status = sender.clone().with(|item| {
    futures::future::ok(Status::Download(item))
});

let unpack_status = sender.clone().with(|item| {
    futures::future::ok(Status::Unpack(item))
});

It would be convenient for With combinator to implement Clone,
since the underlying sink, futures::channel::mpsc::Sender,
implements it.

This change adds #[derive(Clone)] to With combinator

Consider the following use-case of mpsc channels. There are two
categories of producers which produce items of two distinct types,
later unified in one using `With` combinator:

```
let (sender, receiver) = mspc::channel(100);

let download_status = sender.clone().with(|item| {
    futures::future::ok(Status::Download(item))
});

let unpack_status = sender.clone().with(|item| {
    futures::future::ok(Status::Unpack(item))
});
```

It would be convenient for `With` combinator to implement `Clone`,
since the underlying sink, `futures::channel::mpsc::Sender`,
implements it.

This change adds `#[derive(Clone)]` to `With` combinator
@taiki-e taiki-e self-requested a review December 13, 2020 15:35
Copy link
Member

@taiki-e taiki-e 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 the PR. derive is not the better way to do this, as that adds U: Clone, Item: Clone bounds. Could you use manual impl instead of deriving?

@taiki-e taiki-e added C-feature-request futures-util S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author labels Dec 13, 2020
Address review comments

This change

* Implements `Clone` for `With` manually since we don't want to have
  `U: Clone, Item: Clone` bounds.
* Adds a test case for `Clone` implementation.
@akhramov
Copy link
Contributor Author

Thanks for the review.

Could you use manual impl instead of deriving?

Sure thing, please see the update.

Also, I'm not familiar with the code style of the project, so please feel free to point out any slipups.

@taiki-e taiki-e added A-sink Area: futures::sink and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author futures-util labels Dec 17, 2020
Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@taiki-e taiki-e changed the title Derive Clone on With combinator Implement Clone on With combinator Dec 22, 2020
@taiki-e taiki-e merged commit 8a04b51 into rust-lang:master Dec 22, 2020
@taiki-e taiki-e mentioned this pull request Jan 8, 2021
exrook pushed a commit to exrook/futures-rs that referenced this pull request Apr 5, 2021
Consider the following use-case of mpsc channels. There are two
categories of producers which produce items of two distinct types,
later unified in one using `With` combinator:

```
let (sender, receiver) = mspc::channel(100);

let download_status = sender.clone().with(|item| {
    futures::future::ok(Status::Download(item))
});

let unpack_status = sender.clone().with(|item| {
    futures::future::ok(Status::Unpack(item))
});
```

It would be convenient for `With` combinator to implement `Clone`,
since the underlying sink, `futures::channel::mpsc::Sender`,
implements it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sink Area: futures::sink
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants