[franticgecko3]

I'm worried readers of this article will be horrified and believe this kind of DIY error handling is necessary in Go.

The author has attempted to fix their unidiomatic error handling with an even more unidiomatic error framework.

New Go users: most of the time returning an error without checking its value or adding extra context is the right thing to do

[mattgreenrocks]

> New Go users: most of the time returning an error without checking its value or adding extra context is the right thing to do

Thank you.

Feels like Go is having its Java moment: lots of people started using it, so questions of practice arise despite the language aiming at simplicity, leading to the proliferation of questionable advice by people who can't recognize it as such. The next phase of this is the belief that the std library is somehow inadequate even for tiny prototypes because people have it beaten over their heads that "everybody" uses SuperUltraLogger now, so it becomes orthodox to pull that dependency in without questioning it.

After a bunch of iterations of this cycle, you're now far away from simplicity the language was meant to create. And the users created this situation.

[int_19h]

Go is having a Go moment: lots of people using it are realizing that other programming languages have all that complexity for a reason, and that "aiming at simplicity" by aggressively removing or ignoring well-established language features often results in more complicated code that's easier to get wrong and harder to reason about.

[vultour]

From my experience this is not the case. If you error out 7 functions deep and only return the original error there's no chance you're figuring out where it happened. Adding context on several levels is basically a simplified stack trace which lets you quickly find the source of the error.

[richbell]

I agree; I've wasted countless hours troubleshooting errors returned in complex Go applications. The original error is not sufficient.

[mrj]

I inherited a codebase with the same problem. After a few debugging sessions where it wasn't clear where the error was coming from, I decided the root problem was that we didn't have stack traces.

Fortunately, the code was already using zap and it had a method for doing exactly that:

zap.AddStacktrace(zap.LevelEnablerFunc(func(lvl zapcore.Level) bool { return lvl >= zapcore.InfoLevel }))

Because most of the time if there's an error, you'd likely want to log it out. Much of the code was doing this already, so it made sense to ensure we had good stack traces.

There's overhead to this, but in our codebase there was a dearth of logging so it didn't matter much. Now when things are captured we know exactly where it happened without having to do what the post is doing manually... adding stack info.

[mplanchard]

We actually went through the same realization when we started writing Rust a few years ago. The `thiserror` crate makes it easy to just wrap and return an error from some third-party library, like:

    #[derive(Debug, thiserror::Error)]
    enum MyError {
      #[error(transparent)]
      ThirdPartyError(#[from] third_party::Error)
    }

Since it derives a `From` implementation, you can use it as easily as:

    fn some_function() -> Result<(), MyError> {
      third_party::do_thing()?;
    }

But if that's happening somewhere deep in your application and you call that function from more than one place, good luck figuring out what it is! You wind up with an error log like `third_party thing failed` and that's it.

Generally, we now use structured error types with context fields, which adds some verbosity as specifying a context becomes required, but it's a lot more useful in error logs. Our approach was significantly inspired by this post from Sabrina Jewson: https://sabrinajewson.org/blog/errors

[tetha]

It's not a binary decision though. Just because the article arrives at overkill for most things in my opinion doesn't mean sentinel errors or wrapping errors in custom types should be avoided at all costs in all situations.

In my experience, it's good and healthy to introduce this additional context on the boundaries of more complex systems (like a database, or something accessing an external API and such), especially if other code wants to behave differently based on the errors returned (using errors.Is/errors.As).

But it's completely not necessary for every single plumping function starts inspecting and wrapping all errors it encounters, especially if it cannot make a decision on these errors or provide better context.

[mariusor]

Do you maybe have a constructive advice for people that need to return errors that demand different behaviour from the calling code?

I gave an example higher in the thread: if searching for the entity that owns the creds.json files fails, we want to return a 404 HTTP error, but if creds.json itself is missing, we want a 401 HTTP error. What would be the idiomatic way of achieving this in your opinion?

[tetha]

With some of these examples, I'd change the API of the lower-level methods. Instead of a (Credentials, err) and the err is a NotFound sometimes, I'd rather make it a (*Credentials, bool, err) so you can have a (creds, found, err), and err would be used for actual errors like "File not found"/"File unreadable"/...

But other than that, there is nothing wrong with having sentinel errors or custom error types on your subsystem / module boundaries, like ErrCredentialsNotFetched, ErrUserNotFound, ErrFileInvalid and such. That's just good abstraction.

The main worry is: How many errors do you actually need, and how many functions need to mess about with the errors going around? More error types mean harder maintenance in the future because code will rely on those. Many plumbing or workflow functions probably should just hand the errors upwards because they can't do much about it anyways.

A lot of the details in the errors of the article very much feel like business logic and API design is getting conflated with the error framework.

Is "Cannot edit a whatsapp message template more than 24 hours" or "the users account is locked" really an error like "cannot open creds.json: permission denied" or "cannot query database: connection refused"? You can create working code like that, but I can also use exceptions for control flow. I'd expect these things to come from some OpenAPI spec and some controller-code make this decision in an if statement.

[sethammons]

Use errors.Is and compare to the returned err to mypkg.ErrOwnerNotExists and mypkg.ErrMissingConfig and the handler decides which status code is appropriate