Replace errutil with pkg/errors

[mewmew]

Currently we are using errutils at some places, e.g. https://github.com/karlek/flac/blob/66eeb19133306fb37c2cfa5b3dab532224c734f7/enc.go#L28

At other places, we return the error without added context.

I suggest we change from mewkiz/pkg/errutil to pkg/errors and at the same time make sure to add context to all returned errors. What are your thoughts @karlek?

[karlek]

I agree. If we can reduce 15 competing standards to 14, I would be happy to do so.

[karlek]

Replacing is easy, but finding the ones without added context is much more difficult.

[mewmew]

Replacing is easy, but finding the ones without added context is much more difficult.

Could you elaborate, what does "finding the ones without added context" refer to?

I wish to get added info, e.g. include call stack for each error: https://godoc.org/github.com/pkg/errors#hdr-Adding_context_to_an_error

Edit: Ah, I see what you mean now. Rather than using errors,WithStack you refer to adding our own context to the error using errors.Wrap. Yeah, that would be more difficult for sure.

Where it makes sense we can do it, and use WithStack for others.

[mewmew]

Hmm, this may be a breaking change I just realized. Having changed to make use of pkg/errors to add call stack context to error values, then io.EOF is no longer the return value for end of file. So, the check has to be updated from if err == io.EOF to if errors.Cause(err) == io.EOF.

The errors branch fail with:

u@x220 ~/D/g/s/g/m/flac> go test ./...
--- FAIL: TestEncode (0.02s)
	enc_test.go:38: "meta/testdata/input-SCPAP.flac": unable to parse FLAC file; EOF
	enc_test.go:38: "meta/testdata/input-SCVA.flac": unable to parse FLAC file; EOF
	enc_test.go:38: "meta/testdata/input-SCVPAP.flac": unable to parse FLAC file; EOF
	enc_test.go:38: "meta/testdata/silence.flac": unable to parse FLAC file; EOF
	enc_test.go:38: "testdata/19875.flac": unable to parse FLAC file; EOF
	enc_test.go:38: "testdata/8297-275156-0011.flac": unable to parse FLAC file; EOF
	enc_test.go:38: "testdata/love.flac": unable to parse FLAC file; EOF
--- FAIL: TestEncodeComment (0.00s)
	enc_test.go:68: unable to parse input FLAC file; EOF
--- FAIL: TestSkipID3v2 (0.00s)
	flac_test.go:11: EOF
2018/05/25 18:59:08 EOF
FAIL	github.com/mewkiz/flac	0.030s
?   	github.com/mewkiz/flac/cmd/flac2wav	[no test files]
?   	github.com/mewkiz/flac/cmd/go-metaflac	[no test files]

Have to investigate further. Perhaps this update will be part of the move to ljud/flac then, as it is a breaking change.

[karlek]

Wow, that's a concealed footgun I didn't think about. Yeah, sounds good. If we find a solution we'll add it before the big move.

[mewmew]

Wow, that's a concealed footgun I didn't think about. Yeah, sounds good. If we find a solution we'll add it before the big move.

I know, very scary. Especially as this will introduce issues in the future when we forget to be careful about handling the added context in the right way.

I tried to find a solution that allows us to make the transition today, and there is a way. Basically, for each exposed API, we need to make sure that every error value returned is unwrapped if it is either an API global variables (e.g. frame.ErrInvalidSyncCode and meta.ErrReservedType) or a standard library error that we return to implement specific interfaces (e.g. return io.EOF to implement io.Reader).

Having tried to do this, I can testify that this approach is fragile as hell. For instance, lets say we have frame.Frame.Parse return the concrete error values. Then since frame.Parse invokes frame.Frame.Parse, it cannot wrap the error, but also has to introspect it to unwrap if belonging to exposed error values, and otherwise wrap it.

Now, what if we add a new function in a year or two, say frame.ParseFloat that parses the frame using frame.Frame.Parse and converts the PCM samples to floating-point representation. Then we have to remember to keep this error unwrapping in mind. I know for certain that I'll miss it, and thus introduce a rather nasty issue. For instance. our test cases are stuck in infinite loops on the errors branch right now, as they cannot break the loop by identifying io.EOF. (They can of course use errors.Cause to identify io.EOF and break the loop, but that just proves the point, that you have to keep this in mind).

So, for what it's worth. I think we keep it as is for now. I usually don't like the phrase Don't fix it if it aint broken. as even working things can be ugly as hell, and being a hobby project and all, its fun to build something beautiful. But this definitely requires more thought.

Edit: correction, the errors branch works now, with test cases passing and all; and the latest changes of the master branch have been merged into the errors branch. However, this relies on the usage of errors.Cause consistently for comparing errors against their concrete values. I don't mind this approach, and we could document it. But it would be a breaking change.

[pwaller]

Worth mentioning that the go 2 errors proposal was just accepted and due to land in go 1.13. golang/go#29934

[mewmew]

Look into using error return paths (a la Zig), as implemented by for instance https://github.com/bracesdev/errtrace