3 ms·

The error constructor errors.E is without a doubt the worst design for this type of code. It seems Rob Pike is looking for the simplicity of method overloading

The error constructor errors.E is without a doubt the worst design for this type of code. It seems Rob Pike is looking for the simplicity of method overloading here, which Go (which Pike co-designed) explicitly doesn't provide [1]:. "Method dispatch is simplified if it doesn't need to do type matching as well. Experience with other languages told us that having a variety of methods with the same name but different signatures was occasionally useful but that it could also be confusing and fragile in practice. Matching only by name and requiring consistency in the types was a major simplifying decision in Go's type system." So Pike turned to the catch-all interface{} type and allows you to specify anything and everything as a parameter. A few quick questions. Without reaching for the source of the implementation of errors.E, what happens when: 1. You supply two or more Kinds as arguments? 2. Same as above for all other types(Op, Err, PathName, UserName)? 3. You supply a completely random object, string or integer as argument? Couple this with the fact that error-paths are often the least-tested code and you have a recipe for disaster. This is a hack. It might be quick to write but it is certainly not well-designed or elegant. I say that as a proponent of Go. [1] https://golang.org/doc/faq#overloading https://golang.org/doc/faq#overloading

xiaq 9y ago
I wonder why he didn't use the Builder-like approach used in the context package (https://golang.org/pkg/context/ https://golang.org/pkg/context/). It is more verbose to implement, but since this error package is used throughout upspin it sounds like a net win. Edit: the With* functions in context is not exactly Builder-like since some of them return multiple values and cannot be chained. However, the error type doesn't suffer this. You can implement With* methods on the Error type and make the following possible: errors.New("message").WithKind(errors.Permission).WithOp("create") Which is now completely type-safe, and only slightly more verbose than errors.E("message", errors.Permission, errors.Op("create"))
andreareina 9y ago
The docs[1] do say that the last of a particular type wins, though as I say in [2] it's not clear to me whether that's actually desirable, or if it's essentially an implementation detail (process the arguments in order and mutate the object) that got blessed because it's easier that way. I'd love to hear arguments for last-write-wins over immutable assignment. It seems you can't implement custom interfaces on the core data types[3], so a do-nothing interface isn't an option to emulate sum types and enforce type-safety (E switches on string to set the error message). At least the code[4] gives you a run-time error instead of silently ignoring unsupported types. [1] https://godoc.org/upspin.io/errors#E https://godoc.org/upspin.io/errors#E [2] https://news.ycombinator.com/item?id=15870334 https://news.ycombinator.com/item?id=15870334 [3] http://www.jerf.org/iri/post/2917 http://www.jerf.org/iri/post/2917 [4] https://upspin.googlesource.com/upspin/+/master/errors/errors.go#154 https://upspin.googlesource.com/upspin/+/master/errors/error...

xiaq 9y ago

I wonder why he didn't use the Builder-like approach used in the context package (https://golang.org/pkg/context/ https://golang.org/pkg/context/). It is more verbose to implement, but since this error package is used throughout upspin it sounds like a net win. Edit: the With* functions in context is not exactly Builder-like since some of them return multiple values and cannot be chained. However, the error type doesn't suffer this. You can implement With* methods on the Error type and make the following possible: errors.New("message").WithKind(errors.Permission).WithOp("create") Which is now completely type-safe, and only slightly more verbose than errors.E("message", errors.Permission, errors.Op("create"))

andreareina 9y ago

The docs[1] do say that the last of a particular type wins, though as I say in [2] it's not clear to me whether that's actually desirable, or if it's essentially an implementation detail (process the arguments in order and mutate the object) that got blessed because it's easier that way. I'd love to hear arguments for last-write-wins over immutable assignment. It seems you can't implement custom interfaces on the core data types[3], so a do-nothing interface isn't an option to emulate sum types and enforce type-safety (E switches on string to set the error message). At least the code[4] gives you a run-time error instead of silently ignoring unsupported types. [1] https://godoc.org/upspin.io/errors#E https://godoc.org/upspin.io/errors#E [2] https://news.ycombinator.com/item?id=15870334 https://news.ycombinator.com/item?id=15870334 [3] http://www.jerf.org/iri/post/2917 http://www.jerf.org/iri/post/2917 [4] https://upspin.googlesource.com/upspin/+/master/errors/errors.go#154 https://upspin.googlesource.com/upspin/+/master/errors/error...