3 ms·

I agree except for the part about using strings, as you lose structure they way. You should instead return structured errors: return CouldNotStoreUser{

I agree except for the part about using strings, as you lose structure they way. You should instead return structured errors: return CouldNotStoreUser{ UserID: userId, } and now this struct is available to anyone looking up the chain of wrapped errors.

masklinn 1y ago
This is covered by the “ergonomics” section of TFA: > With custom error structs however, it's a lot of writing to create your own error type and thus it becomes more of a burden to encourage your team members to do this. Because you need a type per layer, and that type needs to implement both error and unwrap.
- atombender 1y ago
  In practice, I have found the benefit of explicit typing to outweigh the downsides of needing to declare the types. As a concrete example, it means you can target types with precision in the API layer: switch e := err.(type) { case UserNotFound: writeJSONResponse(w, 404, "User not found") case interface { Timeout() bool }: if e.Timeout() { writeJSONResponse(w, 503, "Timeout") } } I skimmed the article and didn't see the author proposing a way to do that with their arbitrary key/value map. Of course, you could use something else like error codes to translate groups of errors. But then why not just use types? But as I suggested in my other comment, you could also generalize it. For example: return meta.Wrap(err, "storing user", "userID", userID) Here, Wrap() is something like: func Wrap(err error, msg string, kvs ...any) { return &KV{ KV: kvs, cause: err, msg: msg, } } This is the inverse of the context solution. The point is to provide data at the point of error, not at every call site. You can always merge these later into a single map and pay the allocation cost there: var fields map[string]any for err != nil { if e, ok := err.(*KV); ok { for i := 0; i < len(KV.KV); i += 2 { fields[e.KV[i].(string)] = e.KV[i+1] } } err = errors.Unwrap(err) }
  - radlad 1y ago
    You can also just assign your errors to variables, and typically you only need to do so for the innermost error. Wrapping calls can just use Errorf with the %w operator. Horses for courses.
  - kiitos 1y ago
    err.(type) fails when errors are wrapped, which is common and needs to be accommodated. To do what you're trying to do, you need to use errors.As.
    - atombender 1y ago
      Sure. It was not intended to be a complete example.
      - kiitos 1y ago
        It's not that the example is incomplete, it's that it's incorrect! :)
        atombender 1y ago
        No, it's just incomplete. The same code just needs to unwrap: for err != nil { switch e := err.(type) { case UserNotFound: writeJSONResponse(w, 404, "User not found") return case interface { Timeout() bool }: if e.Timeout() { writeJSONResponse(w, 503, "Timeout") return } } err = errors.Unwrap(err) }
        kiitos 1y ago
        `err.(type)` is incorrect, at least in general. Calling `errors.Unwrap` in application code like this is almost always a red flag indicating a design error. And in this case it definitely is!
        1y ago

masklinn 1y ago

This is covered by the “ergonomics” section of TFA: > With custom error structs however, it's a lot of writing to create your own error type and thus it becomes more of a burden to encourage your team members to do this. Because you need a type per layer, and that type needs to implement both error and unwrap.

atombender 1y ago
In practice, I have found the benefit of explicit typing to outweigh the downsides of needing to declare the types. As a concrete example, it means you can target types with precision in the API layer: switch e := err.(type) { case UserNotFound: writeJSONResponse(w, 404, "User not found") case interface { Timeout() bool }: if e.Timeout() { writeJSONResponse(w, 503, "Timeout") } } I skimmed the article and didn't see the author proposing a way to do that with their arbitrary key/value map. Of course, you could use something else like error codes to translate groups of errors. But then why not just use types? But as I suggested in my other comment, you could also generalize it. For example: return meta.Wrap(err, "storing user", "userID", userID) Here, Wrap() is something like: func Wrap(err error, msg string, kvs ...any) { return &KV{ KV: kvs, cause: err, msg: msg, } } This is the inverse of the context solution. The point is to provide data at the point of error, not at every call site. You can always merge these later into a single map and pay the allocation cost there: var fields map[string]any for err != nil { if e, ok := err.(*KV); ok { for i := 0; i < len(KV.KV); i += 2 { fields[e.KV[i].(string)] = e.KV[i+1] } } err = errors.Unwrap(err) }
- radlad 1y ago
  You can also just assign your errors to variables, and typically you only need to do so for the innermost error. Wrapping calls can just use Errorf with the %w operator. Horses for courses.
- kiitos 1y ago
  err.(type) fails when errors are wrapped, which is common and needs to be accommodated. To do what you're trying to do, you need to use errors.As.
  - atombender 1y ago
    Sure. It was not intended to be a complete example.
    - kiitos 1y ago
      It's not that the example is incomplete, it's that it's incorrect! :)
      - atombender 1y ago
        No, it's just incomplete. The same code just needs to unwrap: for err != nil { switch e := err.(type) { case UserNotFound: writeJSONResponse(w, 404, "User not found") return case interface { Timeout() bool }: if e.Timeout() { writeJSONResponse(w, 503, "Timeout") return } } err = errors.Unwrap(err) }
        kiitos 1y ago
        `err.(type)` is incorrect, at least in general. Calling `errors.Unwrap` in application code like this is almost always a red flag indicating a design error. And in this case it definitely is!
        1y ago