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 use of into_future for requests and messages #1003

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

n1ghtmare
Copy link
Collaborator

@n1ghtmare n1ghtmare commented Jun 22, 2023

Supersedes #782

@n1ghtmare n1ghtmare requested review from Jarema and caspervonb June 22, 2023 22:42
@n1ghtmare n1ghtmare self-assigned this Jun 22, 2023
@caspervonb caspervonb changed the title Refactor towards into_future for Client and Context Make use of ìnto_future` for requests and messages Jun 25, 2023
@caspervonb caspervonb changed the title Make use of ìnto_future` for requests and messages Make use of into_future for requests and messages Jun 25, 2023
@n1ghtmare n1ghtmare force-pushed the n1ghtmare/into-future-refactor branch 2 times, most recently from f419f65 to 8c109bc Compare July 1, 2023 09:31
@n1ghtmare n1ghtmare force-pushed the n1ghtmare/into-future-refactor branch from 8c109bc to 21e6620 Compare July 11, 2023 15:26
@caspervonb caspervonb force-pushed the n1ghtmare/into-future-refactor branch from 21e6620 to 06e8c7d Compare July 17, 2023 16:12
@n1ghtmare n1ghtmare force-pushed the n1ghtmare/into-future-refactor branch from 06e8c7d to 3d613b3 Compare July 24, 2023 12:35
Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

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

Mostly test changes requests.

async-nats/tests/jetstream_tests.rs Show resolved Hide resolved
async-nats/tests/jetstream_tests.rs Show resolved Hide resolved
async-nats/tests/jetstream_tests.rs Show resolved Hide resolved
async-nats/tests/jetstream_tests.rs Show resolved Hide resolved
async-nats/tests/jetstream_tests.rs Show resolved Hide resolved
@n1ghtmare n1ghtmare requested a review from Jarema July 25, 2023 14:09
@n1ghtmare n1ghtmare force-pushed the n1ghtmare/into-future-refactor branch from 8e0663b to d7dbf6d Compare July 26, 2023 22:02
@Jarema Jarema force-pushed the n1ghtmare/into-future-refactor branch from d7dbf6d to 78243d1 Compare July 27, 2023 18:50
@@ -1268,6 +1195,106 @@ impl Publish {
}
}

impl IntoFuture for Publish {
type Output = Result<PublishAckFuture, PublishError>;
type IntoFuture = Pin<Box<dyn Future<Output = Result<PublishAckFuture, PublishError>> + Send>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use BoxFuture from futures here?

async-nats/src/client.rs Outdated Show resolved Hide resolved
async-nats/src/client.rs Show resolved Hide resolved

impl IntoFuture for Publish {
type Output = Result<(), PublishError>;
type IntoFuture = Pin<Box<dyn Future<Output = Result<(), PublishError>> + Send>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Also BoxFuture here

async-nats/src/jetstream/message.rs Show resolved Hide resolved
@n1ghtmare n1ghtmare force-pushed the n1ghtmare/into-future-refactor branch from 2498525 to 285222f Compare August 1, 2023 11:28
@Jarema Jarema force-pushed the n1ghtmare/into-future-refactor branch from 285222f to bfd34f9 Compare August 4, 2023 06:39
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