-
Notifications
You must be signed in to change notification settings - Fork 69
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
Expose Http Client #22
Comments
I think exposing the httpClient object is risky because it breaks the encapsulation logic used in the code. Maybe a better approach could be creating an overloaded version of the function I am open to suggestions along these lines :) Thanks, -- @riferrei |
Got it - will set this up now |
Will clean this up, but probably the cleanest I can do (w/o builder pattern since Golang doesn't support virtual method overriding. Alternatively, we can add a |
I was thinking about this last week and had started to add something similar to the following and wasn't quite sure it was the way to go. I ended up backing out of the change and using type Config func(*SchemaRegistryClient)
func WithHttpClient(httpClient *http.Client) *SchemaRegistryClient {
return func (client *SchemaRegistryClient) {
client.httpClient = httpClient
}
}
func CreateSchemaRegistryClient(schemaRegistryURL string, options ...Config) *SchemaRegistryClient {
schemaRegistryClient := ...
for opt := range options {
opt(schemaRegistryClient)
}
return schemaRegistryClient
} |
I think the functional option direction would be best here. I would try steer clear of exposing the http client on the struct to avoid accidental mutation. It also would lead to a bit of a fragmented configuration API imo. |
I'd like to assign myself to this work, and I don't plant to expose the http client. Could you give me a list of what kind of changes user might want to do on the http client? |
Closed as http client is now exposed with #49 |
Certain methods for exposing the registry client (namely service tokens require access to the http client to add auth headers. Would you be open to exposing the
httpClient
->HttpClient
onSchemaRegsitryClient
so we can add in interceptors and inject the headers we need. I'm open to other approaches as well and will be happy to submit a pull request however you want me to implement thisThe text was updated successfully, but these errors were encountered: