-
Notifications
You must be signed in to change notification settings - Fork 215
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
Implement Functional Options #33
Conversation
@italolelis Thanks for the PR. will Check it this week. |
@italolelis There are merge conflicts, can you help resolve these plz. |
@rShetty done |
looking forward to this merge, good job @italolelis |
@italolelis Skimmed through the PR. Overall looks solid and Thank you for this. Have added few comments. |
@rShetty Thanks for reviewing, I can't see the comments that you left though. Do I need to fix something? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes
README.md
Outdated
@@ -259,15 +268,19 @@ func (c *myHTTPClient) Do(request *http.Request) (*http.Response, error) { | |||
} | |||
``` | |||
|
|||
And set this with `httpClient.SetCustomHTTPClient(&myHTTPClient{client: http.DefaultClient})` | |||
And set this with `httpclient.NewClient(httpclient.NewClient&myHTTPClient{client: http.DefaultClient}))` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brackets seem off, can we fix this
// WithHTTPTimeout sets hystrix timeout | ||
func WithHTTPTimeout(timeout time.Duration) Option { | ||
return func(c *Client) { | ||
c.timeout = timeout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add some sane defaults to all of the functional options ?
// WithHTTPTimeout sets hystrix timeout | ||
func WithHTTPTimeout(timeout time.Duration) Option { | ||
return func(c *Client) { | ||
c.timeout = timeout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defaults in these functional options too
// WithRetryCount sets the retry count for the Client | ||
func WithRetryCount(retryCount int) Option { | ||
return func(c *Client) { | ||
c.retryCount = retryCount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also test cases for this file ?
@italolelis Have commented |
@rShetty I've added the defaults and tests. Please, check if it makes sense. |
Thank you very much for the PR |
Hello,
As proposed in here #31, the following implements functional options into the library. This is a breaking change and should probably increase the major version. A few things to consider:
New package names
Why?
I started to have name conflicts with the options that were common to both clients since both of them are in the root folder of the project. Here is an example of the issue:
For this reason, I thought it would be more elegant to have the client implementations in their own package. I called them
httpclient
(to avoid conflict with thehttp
package) andhystrix
Now the usage looks like:
Returning concrete types
Now I started to return concrete types on the constructors of the clients and to make sure the compiler can identify if we are implementing the interface I added this line.
If you don't like the packaging strategy please let me know and we can try to find a solution.
Thanks!