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 ìnto_future` for requests and messages #782

Closed
wants to merge 4 commits into from

Conversation

caspervonb
Copy link
Collaborator

@caspervonb caspervonb commented Dec 23, 2022

  • Makes Client::publish return an into_future based builder
  • Makes Client::request return an into_future based builder.
  • Makes Context::request return an into_future based builder.
  • Makes Context::publish return an into_future based builder.

@Jarema Jarema mentioned this pull request Mar 23, 2023
16 tasks
@caspervonb caspervonb marked this pull request as ready for review March 30, 2023 20:42
@caspervonb caspervonb requested a review from Jarema April 3, 2023 12:38
@Jarema
Copy link
Member

Jarema commented Apr 24, 2023

  1. Fix the conflicts
  2. As this is close to 1.0 changes, maybe we can have kv hotpath operations here too?

@paolobarbolini
Copy link
Contributor

Having the IntoFuture implementations return BoxFutures looks very inefficient

@caspervonb
Copy link
Collaborator Author

caspervonb commented Apr 24, 2023

Having the IntoFuture implementations return BoxFutures looks very inefficient

Aye, inferring the type without boxing will help here. But it's still nightly.

@paolobarbolini
Copy link
Contributor

Having the IntoFuture implementations return BoxFutures looks very inefficient

Aye, inferring the type without boxing will help here. But it's still nightly.

If they come out when you're in 1.0 though you'll have to make a breaking release. I could look into manually implementing the Future for it.

}

impl Publish {
pub fn new(sender: mpsc::Sender<Command>, subject: String, payload: Bytes) -> Publish {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be public?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does not.

@Jarema
Copy link
Member

Jarema commented Jun 14, 2023

@caspervonb I would put it in queue for final review and merge after concrete errors for JetStream are merged, so we have only one conflict solving step.

@caspervonb
Copy link
Collaborator Author

Quite a few conflicts in this one, closing in favor of #1003

@caspervonb caspervonb closed this Jul 2, 2023
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.

None yet

4 participants