Pull Request Test Coverage Report for Build 7652203560

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.2%) to 55.481%

Totals
Change from base Build 7358487704:	0.2%
Covered Lines:	2647
Relevant Lines:	4771

💛 - Coveralls

The problem with this implementation is that the ClientOptions struct becomes part of our public API, limiting how we can change it in the future. The Uber method is definitely more cumbersome to implement, but allows us to keep it private.

Thanks for the link @jhillyerd

Actually, I can just make ClientOpts small and then it is not exposed, and still works. It looks like ubers argument is:

that there's a method of implementing this pattern with closures but we believe that the pattern above provides more flexibility for authors and is easier to debug and test for users. In particular, it allows options to be compared against each other in tests and mocks, versus closures where this is impossible. Further, it lets options implement other interfaces, including fmt.Stringer which allows for user-readable string representations of the options.

I can change from the pattern with closure to the one you and uber use with apply 👍

Looks like they used to use the closure approach but changed uber-go/guide#74

Looks good, but apologies, I should have mentioned this before: What do you think about dropping the Client name prefix from most of the types/funcs you are introducing? I know the existing client.Client type has repetition in it, but I think we could use client.Option interface and client.WithOptTransport instead.

Looks good, but apologies, I should have mentioned this before: What do you think about dropping the Client name prefix from most of the types/funcs you are introducing? I know the existing client.Client type has repetition in it, but I think we could use client.Option interface and client.WithOptTransport instead.

Good point.... client.client* is repetitive, I also thought about changing but didnt for some reason 😄 .... will adjust 👍

Thanks a bunch!