Skip to content

Comments

(#796) User should be responsible for request encoding with client endpoint#916

Closed
LasTshaMAN wants to merge 2 commits intogo-kit:masterfrom
LasTshaMAN:user-should-be-responsible-for-request-encoding-with-client-endpoint
Closed

(#796) User should be responsible for request encoding with client endpoint#916
LasTshaMAN wants to merge 2 commits intogo-kit:masterfrom
LasTshaMAN:user-should-be-responsible-for-request-encoding-with-client-endpoint

Conversation

@LasTshaMAN
Copy link

@LasTshaMAN LasTshaMAN commented Sep 28, 2019

TODO:

  • check out whether some docs require updating as well (so that they will describe new API usage instead of the deprecated one)
  • increase test coverage (especially check the part about Content-Length)

@peterbourgon
Copy link
Member

Thanks, I'll work on the build failures.

@peterbourgon
Copy link
Member

Can you rebase on master?

@LasTshaMAN
Copy link
Author

Sure I can rebase, will do soon!

@LasTshaMAN LasTshaMAN force-pushed the user-should-be-responsible-for-request-encoding-with-client-endpoint branch from 4d9fbd4 to 9fd881c Compare October 3, 2019 21:28
@LasTshaMAN
Copy link
Author

Rebased.

@LasTshaMAN
Copy link
Author

@peterbourgon, have you had a chance to take a look yet ?

@peterbourgon
Copy link
Member

Sorry for the delay. We can't break the transport/http API by removing the Client type. Can you not incorporate the changes into that type?

@peterbourgon
Copy link
Member

More concretely, I would want to see EncodeClientRequestFunc (probably better named CreateRequestFunc) as a field in the Client type, with a default implementation that has the current behavior (http.NewRequest) and an Option to set it to something else.

@LasTshaMAN
Copy link
Author

LasTshaMAN commented Nov 6, 2019

We can't break the transport/http API by removing the Client type

I think this PR does not break it, only deprecates it (at least this is how I intended it to be). And if // Deprecated annotation bothers you, we can remove it, of course :)

Can you not incorporate the changes into that type? More concretely, I would want to see EncodeClientRequestFunc (probably better named CreateRequestFunc) as a field in the Client type, with a default implementation that has the current behavior (http.NewRequest) and an Option to set it to something else.

We can go this route, but I see the following major obstacle, we initialize Client like this:

func NewClient(
	method string,
	tgt *url.URL,
	enc EncodeRequestFunc,
	dec DecodeResponseFunc,
	options ...ClientOption,
) *Client {
...

that is, we require the API consumer to pass in enc EncodeRequestFunc.

So, as API consumer if I want to create my requests through CreateRequestFunc option (like you are suggesting above) rather than through EncodeRequestFunc - what value for enc should I pass in NewClient ?

@LasTshaMAN
Copy link
Author

LasTshaMAN commented Jan 20, 2020

Hey, guys, can we follow-up on this ? (it's been some time ...)

@ppacher
Copy link

ppacher commented Mar 17, 2020

Hi, is there any progress here? I'd be cool to get this merged.

@peterbourgon
Copy link
Member

I'm sorry for my lack of action on this PR, I simply haven't had the mental bandwidth to devote to this actually somewhat substantial change. I will make time before the end of the week to get it into shape and merged.

@LasTshaMAN — in the interest of expediency, would it be OK with you if I made a PR to your PR to simply implement whatever changes I felt were appropriate?

@LasTshaMAN LasTshaMAN deleted the user-should-be-responsible-for-request-encoding-with-client-endpoint branch March 21, 2020 05:29
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