Drop multierr module and rely on the standard lib

[twz123]

Description

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update

How Has This Been Tested?

• Manual test
• Auto test added

Checklist:

• My code follows the style guidelines of this project
• My commit messages are signed-off
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules
• I have checked my code and corrected any misspellings

[kke]

There's a ; there.

I suppose defer func() { err = errors.Join(err, in.Close()) } would do the same except if the %w: %w formatting is important.

[twz123]

Not exactly the same. This is touching some of the things I preferred in the multierr implementation.

1. multierr used "; " to join error messages, while errors.Join(...) uses "\n", which has puzzled me since its inception. So yes, the semicolon is intentional, as this is two equitable errors, not one that's been caused by another one, where a colon would be more appropriate.
2. errors.Join(...) wraps the error in any case, even if there's only one, so errors.Join(nil, err) != err, whereas multierr did not wrap a single error: multierr.Combine(nil, err) == err. Same for multierr.Append(...). I wonder why upstream decided to do it that way. Maybe because of 3.
3. I wonder why there isn't a public error type for "a slice of errors", which is basically what errors.Join(...) represents. To me, there's a fundamental difference between "multiple errors occurred" (which is "a slice of errors" / errors.Join(...)) and "an error occurred that has multiple causes" (which is fmt.Errorf(...) with multiple %w placeholders, or some other custom error type that implements Unwrap() []error). The way the Go standard library does it makes these cases indistinguishable.

Those things bother me in the way that things bother people even when they know they really shouldn't care that much 🙈

[twz123]

Done.

[kke]

Go docs say:

When adding additional context to an error, either with fmt.Errorf or by implementing a custom type, you need to decide whether the new error should wrap the original. There is no single answer to this question; it depends on the context in which the new error is created. Wrap an error to expose it to callers. Do not wrap an error when doing so would expose implementation details.

As one example, imagine a Parse function which reads a complex data structure from an io.Reader. If an error occurs, we wish to report the line and column number at which it occurred. If the error occurs while reading from the io.Reader, we will want to wrap that error to allow inspection of the underlying problem. Since the caller provided the io.Reader to the function, it makes sense to expose the error produced by it.

In contrast, a function which makes several calls to a database probably should not return an error which unwraps to the result of one of those calls. If the database used by the function is an implementation detail, then exposing these errors is a violation of abstraction. For example, if the LookupUser function of your package pkg uses Go’s database/sql package, then it may encounter a sql.ErrNoRows error. If you return that error with fmt.Errorf("accessing DB: %v", err) then a caller cannot look inside to find the sql.ErrNoRows. But if the function instead returns fmt.Errorf("accessing DB: %w", err), then a caller could reasonably write

err := pkg.LookupUser(...)
if errors.Is(err, sql.ErrNoRows) …

At that point, the function must always return sql.ErrNoRows if you don’t want to break your clients, even if you switch to a different database package. In other words, wrapping an error makes that error part of your API. If you don’t want to commit to supporting that error as part of your API in the future, you shouldn’t wrap the error.

It’s important to remember that whether you wrap or not, the error text will be the same. A person trying to understand the error will have the same information either way; the choice to wrap is about whether to give programs additional information so they can make more informed decisions, or to withhold that information to preserve an abstraction layer.

So, in that light, %v is sometimes better than %w and errors.Join is not a replacement for adding context to error messages 🤔

[github-actions]

The PR is marked as stale since no activity has been recorded in 30 days