panic() variant of Set() for config about go-nsq HOT 8 CLOSED

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

from go-nsq.

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

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

from go-nsq.

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())
}

from go-nsq.

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

from go-nsq.

@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.

from go-nsq.

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?

from go-nsq.

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().

from go-nsq.

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

from go-nsq.