fix: spinner wraps original error

[Integralist]

Some errors are lost if the spinner itself has an error. This PR ensures a spinner error wraps the original error.

It also provides a spinner.WrapErr() abstraction.

[awilliams-fastly]

I find it a bit confusing how spinErr is checked for nil, but then appears not to be used in the returned error. After inspection, I see that the error is assigned to spinner and is in fact returned via WrapErr. I think this may lead to misuse/confusion. I wonder if we should consider alternatives? Is the goal for the spinner error and the "original" error both to be wrapped and returned, so that they can both be checked for via errors.Is/As?

[Integralist]

I find it a bit confusing how spinErr is checked for nil, but then appears not to be used in the returned error.

Yup, I can see how that might be confusing. That is something I hadn't considered.

After inspection, I see that the error is assigned to spinner and is in fact returned via WrapErr.

Correct. The actual logic error (not the spinner error) is returned via the call to spinner.WrapErr() as we're wrapping both the original error, and the spinner error inside of a new error message.

I wonder if we should consider alternatives?

I'll have a think about it, but do you have any ideas off the top of your head that you think would be useful?

Essentially I want to avoid having to have the following written everywhere...

spinErr := spinner.StopFail()
if spinErr != nil {
  return "", nil, fmt.Errorf("failed to stop spinner (error: %w) when handling the error: %w", spinErr, err)
}

Of course I already (in this PR) have the format string defined as a constant (SpinnerErrWrapper) so maybe I just go back to what I had before I implemented WrapErr(), which was...

spinErr := spinner.StopFail()
if spinErr != nil {
  return "", nil, fmt.Errorf(SpinnerErrWrapper, spinErr, err)
}

Maybe WrapErr() is just a little too magic and we keep with the constant.

Is the goal for the spinner error and the "original" error both to be wrapped and returned, so that they can both be checked for via errors.Is/As?

Summary: I don't think we'd ever care about checking the error with errors.Is/As to see if it's a spinner error.

To clarify my thinking here...

The spinner error isn't a useful message for the user but is useful for a CLI developer (i.e. Fastlyan) because it allows us to notice that we've messed up the logic flow somewhere.

In my experience the spinner only ever fails if you try to call (for example) .Stop() or .StopFail() when .Start() was never called (or vice-versa). In the CLI this happened a lot when I first implemented the spinner, because I was passing a spinner instance around through multiple function layers and it was very easy to call .Stop() at a point in time when .Start() hadn't been called, simply due to how the logic flows through the CLI and sometimes you want a spinner and sometimes, like when passing --verbose, you don't.

Errors rarely happen with the spinner nowadays as I'm more aware of carefully starting/stopping it, but that doesn't mean the spinner won't ever fail because it's very possible I (or someone else) will miss something in future. So the error is important for CLI devs (i.e. me) to know I've messed something up. For the user it's not useful, there's no remediation or anything they can do other than just report the error to Fastly.

So, I don't think we'd ever care about checking the error with errors.Is/As.

[awilliams-fastly]

Thanks for the context, that's helpful.

Even though there's more repetition now, I think it reads better and will be easier to understand.