panic() variant of Set() for config

[tj]

I'm getting IO timeouts with nsqd v0.2.28 (built w/go1.2.1) for anything blocking for a second, which I'll need to fix anyway, but more importantly I didn't realize that Set() returns instead of a panic. I probably just don't have master with the msg_timeout support, but having a MustSet would be great for simple bootstrapping, dev error in my case. yay/nay?

[tj]

also realized the client doesn't reconnect for some reason, nothing to report log-wise

[mreiferson]

Are you using lookupd? If not, it uses a simple 15 second timeout after disconnection.

I'd be fine adding MustSet, go for it.

[jehiah]

If you want to panic on errors you can just do the following. I normally think of Must* methods as not panicing, but being guaranteed to return. Since we are in library mode i think it's generally Go style to prefer errors over panic's (though there are a few cases where we use them).

TL;DR; i'm not sure there is a strong enough reason for a second function to Set config values.

if err := cfg.Set(...); err != nil {
   panic(err.Error())
}

[tj]

Ah I thought the pattern was must return or panic, still a Go noob. I'm not using lookupd in this case, and I just remembered this morning that I've had errors from my local dev nsqd saying msg_timeout was too high so it can't be that, I'll sift through with --verbose

[mreiferson]

@visionmedia you're right, that is the pattern (return or panic). @jehiah we kinda screwed this up in our go-simplejson API, which isn't actually consistent with the Go standard library's use of Must.

Anyway, it is debatable whether having another set method would do the trick. I am sympathetic to the idea that the library should try to help you do the right thing as easily as possible, but having two methods might not be the path forward.

I feel like, at one point, I had written Set to panic, but there are other issues with a library that panics unexpectedly.

Really, the golden rule is to always always always check returned errors when present.

[chris-baynes]

I just came across a similar issue trying to set lookupd_poll_interval to less than the minimum allowed. Not realising Set returned an error, my value was silently ignored.

I see there is also a public Validate method that's used by the producer and consumer to check config validity. Wouldn't it then be better to just set the potentially invalid values, then just return an error from Validate?

The reason I prefer this approach is that wrapping each Set in an if err != nil is quite verbose when a few values need to be set, this would also mean the values are only validated once.

What do you guys think?

[jehiah]

In #52 we re-exported Config values for backwards compatability, and to maintain a type safe way to set config values (or at least one that is caught at compile time). That leaves two ways to set config values; one is using the config struct directly which is ideal for Go code, and the second, through Set() is ideal for parsing config files, or text command line arguments.

Because we allow the former it's possible to set out-of-range values on some items so Validate() asserts that everything is within the allowable range for some config values.

I agree that error handling can be verbose, but it's the Go way, and I find the more I accept that up front I avoid unexpected states in the future. (I'll also say that was a particular style i was personally slow to adopt switching to Go from Python).

To take direction from the stdlib, If os.Hostname() returns an error to indicate there are obscure cases where it can fail (instead of panicing), that pattern that applies to Config.Set().

[tj]

I'm cool with the behaviour, just missed the error. I'll just check(sub.Set()) in my case