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

feat: make RequestBuilder::request public #1736

Closed
wants to merge 1 commit into from

Conversation

Colerar
Copy link

@Colerar Colerar commented Jan 20, 2023

@lucab
Copy link
Contributor

lucab commented Mar 9, 2023

@seanmonstar how do you feel about landing this enhancement? I do have a similar usecase to what is shown in #1212, and I'd be happy to see this PR merged (or some similar logic to reach into the inner Request from the builder).

@seanmonstar
Copy link
Owner

Hm. While it's slightly more of a dance, I think a different approach would be if you could "build" the request, and then turn it back into a builder. That would at least mean we don't have to give out a &Result<..>, essentially exposing the internals of the builder.

@lucab
Copy link
Contributor

lucab commented Mar 9, 2023

Thanks for the quick feedback, personally that alternative would work for me as well.
Just to be explicit, if I correctly understood your idea then we'd need two additional methods to split/reassemble the builder:

  • RequestBuilder::build_split(self) -> (Client, Result<Request>)
  • RequestBuilder::from_parts(Client, Request) -> Self

Or, alternatively to the build_split() above, use the existing build() plus a new clone_client() in order to obtain the inner Client.
(All names and signatures open to bikeshedding)

@seanmonstar
Copy link
Owner

Something to that effect, yes. The only thing that makes me hesitate is "splitting" out the client. Would you no longer have a client to send the built request with? Hm, I suppose you might not, since the builder so far has just bundled it with it...

@lucab
Copy link
Contributor

lucab commented Mar 9, 2023

In my case indeed I don't have otherwise access to the Client.
I think it's kinda common to expose hooks for external logic in the form of ext_hook(rb: RequestBuilder) -> RequestBuilder methods. One such example is the request_builder_hook on https://docs.rs/deno_fetch/0.115.0/deno_fetch/struct.Options.html.

@lucab
Copy link
Contributor

lucab commented Mar 9, 2023

I've posted an implementation of the alternative approach at #1770.

@lucab
Copy link
Contributor

lucab commented Mar 10, 2023

#1770 just landed (thanks @seanmonstar for the quick review!), I think this can be closed.

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

3 participants