Replacement of pkg/errors with stdlib errors

[Eriner]

Removes pkg/errors in favor of stdlib. Referenced in discussion here: #13 (comment)

Script below is lazy, not "feature complete", and comes with caveats.

#!/usr/bin/env zsh

if [[ ! -e ${PWD}/.git/HEAD ]]; then
  echo "directory is not a git repo, probably not safe to run this here! Aborting."
  exit 1
fi

echo "replacing 'pkg/errors' with stdlib 'errors' for:"
_go_errors_files=($(grep -lr 'pkg/errors' . | grep '.go$'))
for file in ${_go_errors_files[@]}; do
  echo " - ${file}"
  sed -E -i '' 's!errors.Wrap\((.*), "(.*)"\)!fmt.Errorf\("\2: %w", \1\)!g' ${file}
  sed -E -i '' 's!errors.Wrap\((.*), (.*)\)!fmt.Errorf\("\2: %w", \1\)!g' ${file}
  sed -E -i '' 's!errors.New\((.*)\)!fmt.Errorf\(\1\)!g' ${file}
  sed -E -i '' 's!errors.Errorf!fmt.Errorf!g' ${file}
  sed -E -i '' 's!errors.Cause\((.*), (.*)\)!errors.Is\(\1, \2\)!g' ${file}
  sed -E -i '' 's!errors.Cause\((.*)\) == (.*) {$!errors.Is\(\1, \2\) {!g' ${file}
  sed -i '' 's!"github.com/pkg/errors"!!g' ${file}
  goimports -w ${file}
done

As for the caveats, Wrap implicitly returns nil
on a nil error, which fmt.Errorf does not. And Cause implies a for-loop
to identify the underlying-most (non-nil) error.

Needs a second set of eyes, as this touches a lot of files with repetitive changes.

[Eriner]

This is the kind of thing to look for.

errors.Wrap() made it easy to execute a function (s.writeMessage()) while ensuring err will remain nil (if no error is encountered by s.writeMessage()).

errors.Wrap() can't be easily replaced with a fmt.Errorf() in cases like this. That is to say, the script as written will break things like this.

[lukechampine]

yep. I definitely got a little "terse-happy" here. I would rewrite this without the named return:

if err := s.writeMessage(&rpcID); err != nil {
    return fmt.Errorf("WriteRequestID: %w", err)
}
if req != nil {
    if err := s.writeMessage(req); err != nil {
        return fmt.Errorf("WriteRequest: %w", err)
    }
}
return nil

[Eriner]

I also don't think this implementation is correct, as errors.Cause() doesn't mean the lowest error in the stack necessarily, an the package implementation returns the topmost error that implements the interface... Someone with a bigger brain than me may need to fix this (in context of this package).

[lukechampine]

I think this is what errors.As is for:

if errors.As(err, new(*renterhost.RPCError)) {
    return fmt.Errorf("%s: %w", rejectCtx, err)
}

(The double indirection is weird, but correct; As wants a pointer to a value that implements error, and in this case renterhost.RPCError has a pointer receiver. Furthermore, since we don't need to actually inspect the error, we don't need to declare a separate variable for it.)

[jkawamoto]

I prefer pkg/errors instead of errors because it gives a stuck trace. This library is still beta, and we need to debug it. I think if it reaches v1, it's a good time to consider replacing pkg/errors with error if necessary.

[lukechampine]

Thanks for the PR! Always nice when a dependency is no longer needed. Given that this was largely automated, I'll go through each case carefully.

I prefer pkg/errors instead of errors because it gives a stack trace

As long as we properly wrap each error, it shouldn't be too difficult to determine the error path. Maybe it's just easy for me because it's my codebase, though. 😅
It's a shame that fmt.Errorf doesn't have a %ws verb or something for including a stack trace.

[Eriner]

I checked most of the conditions manually, but again there are some nuances here in the handling of nil errors.

As long as we properly wrap each error, it shouldn't be too difficult to determine the error path

I usually find stack traces unproductive (as it typically just points to where the error was raised, which is what proper annotation does anyway), but that may just be an artifact of the types of code errors I tend to give birth to.

[lukechampine]

my preference is to stick with errors.New for simple string errors. It adds an extra import, but imo it communicates intent a bit more clearly.

[Eriner]

By intent, do you mean the fact that this is an error cannot be handled internally? If so, does it being exported not imply that?

[lukechampine]

nah, I mean when I'm reading someone's code and see errors.New I think "okay, this is just an opaque string", but when I see fmt.Errorf I think "oh, this must be a fancier error that requires formatting or wrapping."

[Eriner]

I guess somewhere down the line I joined the printf-forever team and it corrupted my brain. That makes much more sense.

[lukechampine]

var hes HostErrorSet
if !errors.As(err, &hes) {
    t.Fatal("expected HostSetError, got", err)

[Eriner]

I figured this one was coming, but I glossed over the As docs, saw "something something panic if something", and tried to maintain the existing implementation as much as possible. Will fix.

[lukechampine]

okay, done. btw, if you didn't know, you can do simple replacements with gofmt. Namely:

gofmt -r 'fmt.Errorf(a) -> errors.New(a)'

I think that will only replace the calls where only one argument is passed. You'll also need to run goimports again afterward.

[Eriner]

All comments should be addressed.

[lukechampine]

LGTM. Can you run go mod tidy and squash into one commit as all: Replace pkg/errors with stdlib? I can't allow commits named flup in my project, sorry :P

[Eriner]

Ha, I wanted to "show my work" expecting that you'd squash on merge. I tidy'd, squashed.

[lukechampine]

Definitely appreciated! The reason I asked is that my CI bot doesn't support squashing yet. Maybe someday.

Alright Goober, test and merge.

[goober-the-friendly-robutt]

I support squashing your head in a vice, meatbag. Maybe someday I'll do that.

-- oh, why hello there, @Eriner! So nice to see a new face around here. Let's put your code to the test...