-
Notifications
You must be signed in to change notification settings - Fork 88
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
Private configurations using configuration function arguments #48
Conversation
Personally I do prefer the builder pattern, because the same builder can be reused for different clients with slightly different configurations. But I'm down to try this approach, since it's definitely better than what we have currently. Could you pull the rate limiter stuff out and send it in a different PR? I'd like to discuss that separately. |
Also in the interest of keeping the PR more readable for review, could you leave the client.go code in analytics.go for now? |
31c1cfb
to
f33cda6
Compare
Ahh, yes. It did feel like a scope creep, didn't it? I removed the ratelimit changes, and pushed it. Do you still want the config in a separate file? |
Oh, and would a final commit reverting the client.go refactor do, or do you want it to disappear? On a separate note, how would you imagine the builder pattern in this context? |
Pushing the revert as final commit, it is 1 am here, so I need some sleep. Pending your reply, I'll take appropriate action tomorrow. I won't be hurt if you feel the need to jump in and do the rebasing yourself if you need to. |
@f2prateek there's also the option of just doing a
|
b5ade0c
to
0a0dfd8
Compare
If I may chime in here, this code construct seems pretty heavy for what it attempts to do and I've never seen this approach in go code. Something that I've often seen on the other hand is having a How would you feel about introducing a configuration object instead? type Config struct {
Key string
Endpoint string
Interval time.Duration
Size int
Verbose bool
}
func NewWithConfig(c *Config) *Client { ... } Then we could create client objects this way: client := analytics.NewWithConfig(&analytics.Config{
Key: "write-key",
...
}) I just wanted to discuss and get your opinion on it. PS: Note that often with this construct passing |
If we did do the config approach, I'd personally prefer writeKey as an explicit argument. type Config struct {
Endpoint string
Interval time.Duration
Size int
Verbose bool
}
client := analytics.NewWithConfig("write-key", &analytics.Config{
...
})
client := analytics.New("write-key") |
What is the reason for that? |
I think having the writeKey as part of the constructor makes a lot of sense, since that is the singular requirement for the client to work. By leaving it in a config, it alludes to the option of leaving it out. We could of course panic if |
What if we were picking up the write key (or other config values) from environment variables or a config file the way AWS does it with its SDKs? http://docs.aws.amazon.com/sdk-for-go/api/ |
Yup - it's to convey that the write key is absolutely required, and the client will not work without a writeKey. It also gives an extra layer of compile time safety. e.g. func NewWithConfig(config Config)
client := analytics.NewWithConfig(analytics.Config{
// might miss write key
...
}) vs. func NewWithConfig(writeKey string, config Config)
// not possible to forget write key as an argument. will not compile without the write key parameter.
client := analytics.NewWithConfig("write-key", analytics.Config{
...
}) Admittedly it is still possible to pass in an empty string |
@achille-roussel aws-sdk-go is actually using the functional-options pattern that I've employed in their aws.Config Their change, is that this is configuring the config, and not some code. The reason I introduced this, was to prevent the close issue, and because I obviously have some love for this particular pattern. If you're interested in the origin, it was Rob Pike that introduced it, and I learned of it from Dave Cheney's blog. But for the purposes of this, I don't feel strongly for either approach. The real purpose was to prevent the blocking I'm actually a massive fan of retrieving this info from either a .segment, environments or direct variable passing. The aws-sdk-go library does this through helper functions that help you specify where you want it from (say, passing it explicitly for testing purposes, where having your local segment writekey win would be unfortunate). |
It is 11 PM here (Denmark), heading to bed. Will check in tomorrow! |
Alright, today I learned something, thanks for the details ;) I'm gonna try out some code samples with different approaches, thanks for your help on that, Good night man! |
So if I understand well, here are the options: package main
import "time"
func main() {
client := analytics.New(
"write-key",
analytics.Interval(2*time.Second),
analytics.Verbose(true),
)
client := analytics.NewWithConfig(
"write-key",
analytics.Config{
Interval: 2 * time.Second,
Verbose: true,
},
)
} I see what's sexy in the first approach, what I don't like is it requires using a lot of top-level symbols to provide all the configuration functions which could easily evolve into name conflicts as the package grows. Godoc is going to group them together since they'll be returning the same type so it's not so hard to discover what are the available settings, but it is much heavier in the implementation (each function is ~8 lines vs 1 line for the field of a structure, which is way more declarative). I also feel like using the config object makes the code easier to read as gofmt presents things nicely aligned. One last point in favor of the config object approach I'm seeing is it's a lower-level abstraction (probably why AWS uses the function approach to generate the config, also it abstracts taking the address of the values because they use pointers everywhere :/ ), if it's interesting for your program to have a functional approach you can still make a config object constructor that follows this pattern and you have code that looks like this: client := analytics.NewWithConfig(
"write-key",
makeAnalyticsConfig(
analyticsInterval(2*time.Second),
analyticsVerbose(true),
),
) What are you guys' thoughts on this? |
I think what I like about the function approach, is that it involves makes all the validation in the function, instead of the constructor, where we would otherwise place the validation. The lines of code argument I feel is rather weak, but I do see the declarative argument. If we are on the configuration approach, why not just have a constructor like so: // Notice private fields
type Config struct {
endpoint string
interval time.Duration
size int
verbose bool
client *http.Client
log *log.Logger
}
func NewConfig() *Config {..} And then use the builder pattern that @f2prateek proposed, having each function return a func (c *Config) Verbose() *Config {
c.verbose = true
return c
} The final constructor as a complete example, would look like so: package main
import "time"
func main() {
config := analytics.NewConfig()
.Verbose()
.FlushInterval(2 * time.Second)
client := analytics.NewWithConfig(
"write-key",
config
)
} For both this approach, and the one you are proposing, we need to be careful about meaningful default values (eg. Size == 0 would, in the current implementation, result in nothing being sent - although I might be mistaken). We could add a error field to the Config struct, and have that be a sticky error though. Each function would then do validation on the input, and set the error accordingly, and potentially skipping entirely if the error is already set: type Config struct {
endpoint string
interval time.Duration
size int
verbose bool
client *http.Client
log *log.Logger
err error
}
func (c *Config) WithBufferSize(size int) *Config {
if c.err != nil { return c }
if size <= 0 {
c.err = fmt.Errorf("WithBufferSize: %d is not a valid size", size)
}
return c
} Alternatively, we could make the values pointers, and only respond to the ones that are |
Oh, and if we are afraid of name-collisions with future stuff, which I find a pretty valid concern, we could adopt the approach that aws-sdk-go did, by prefixing the config functions with func WithBufferSize(size int) ClientConfigFunc {..}
func WithVerbosity() ClientConfigFunc {..} I did that in the example above for the builder pattern configuration without thinking actually.. :) |
017afcd
to
a30ee5c
Compare
d1b7305
to
db32035
Compare
I'm still having trouble seeing what's wrong with this: client, err := analytics.NewWithConfig("write-key", analytics.Config{
...
}) This syntax is easy to read, write, maintain and document, I'm not sure what using methods or functions to build or describe the configuration brings to the table here. I see no problem with doing the configuration check in the constructor, that's where the error has to be returned anyway (even if it's detected earlier as you described in your example) and it can be simply implemented with code like this: func NewWithConfig(k string, c Config) (client *Client, err error) {
// Sets zero-values to defaults, checks that the fields all have acceptable values.
if c, err = checkConfig(c); err != nil {
return
}
...
} What do you think? |
It just different approaches is all, the benefit of the functional approach, is that you don't have to put any thought into which of the values are intentional by the user, and which were not. You only have to validate the functions that are passed, not if all the other configurations are valid or not. Your approach is perfectly valid, as witnessed by the approach being a longstanding pattern - I just like the other one better :) I'd go with whatever you like better. |
Some of the changes in the PR have been addressed by different pull requests, part of it will be in the 2.1.1 release and others in the 3.0.0, I think we'll close this one then. Feel free to reopen it if you feel like we should look back at it. Again, thanks for your contributions! |
Hey segment!
I saw the ongoing work on fixing issue #43 in pull-request #44 and decided to propose my change in the form of my own PR :)
Now, it is a bit of a scope creep, but I also added a ratelimiter with a default of 50 req/sec. This addition and all previous public attributes on the client -
verbose
,client
,interval
andsize
andendpoint
- have been made private and the configuration of these have been replaced with optional configuration functions on the constructor:An example of a configuration function is configuring the
FlushInterval
:And the
New
function, using this configuration would look like so:Inspired by this article by Rob Pike in 2014