Implemented support for checkable errors

[PatrLind]

This is a possible fix for issue #86.

I have added two custom error types that can be used to check if an error is coming from the uuid package.
Not sure if the package maintainers agree with my implementation, but feel free to tell me if it is suitable or not.
Also not sure if there needs to be two errors types. Perhaps one is good enough?

No error messages should be different from before. err.Error() should return the same string.
It should now also be possible to check for uuid errors by using the errors.Is() and errors.As() functions.

I added a test function to verify that the implementation is OK.

[dylan-bourque]

Personally, I'm not a fan of exporting custom error types. What's the use case for needing to determine whether or not an error was generated by this package?

As it's written today, any error returned by any function/method is, by definition, generated by this package since all of them are constructed directly. There's no need for anyone to use errors.Is() or errors.As() to check that.

[PatrLind]

Well... I had a need for it and saw that no one had done anything to fix the bug that was reported 4 years ago.
My use case is that I have an error handler for gRPC that checks for some types of errors that can be returned. One of the errors I check is if the UUID could be parsed correctly. If it was not parsed correctly I send can send back the error to the user in clear text. Otherwise I just send a generic Internal Server Error. So this is a way to filter out internal errors that should not be sent back to the user while letting some specific errors thru. And since the UUID comes from the caller this is important information to know (that the parsing failed).
Right now I need to check if the error string contains "uuid: in" or "uuid: UUID must". This is a bit brittle, and it seems to me that looking for strings in an error message in order to determine if they are from the goofr/uuid package is not really a good practice.

In my experience most packages (at least the ones I use) export errors I can check against with errors.Is().

I'm not exactly sure what you mean by "any error returned by any function/method is, by definition, generated by this package since all of them are constructed directly"?

[dylan-bourque]

I see. Your code is not parsing/decoding the UUID values directly but you want to be able to detect that the error returned by gRPC came from gofrs/uuid, right?

If that's the case, I would personally define a couple of sentinel errors and refactor those fmt.Errorf() calls to to return them. I've kind of decided that I don't like to include "extra" information (like the values of passed-in parameters) in error messages, but not everyone agrees with that.

// alias for string so that we can declare "constant" errors
// see https://dave.cheney.net/2016/04/07/constant-errors
type Error string
func (e Error) Error() string { return string(e) }

const (
    ErrInvalidDataLength = Error("uuid: ....")
    ErrInvalidFormat.    = Error("uuid: incorrect UUID format in string")
)
...

return ErrInvalidFormat
-- OR --
return fmt.Errorf("%w: %q", ErrInvalidFormat, s)

Consumers could still use errors.Is(err, uuid.ErrInvalidFormat) to determine which kind of error happened without introducing the extra complexity of a local error type "hierarchy".

[PatrLind]

Sure, declaring pre-allocated errors like in your example would have been great!
But since I'm pretty sure people have come to depend on the exact error strings in may ways I don't know about, so the current error messages cannot change (the string value returned from Error()). And since a lot of the error messages are including details on how the parsing failed, so those cannot be pre-allocated like that (right?).

I can refactor my code a bit and try to simplify it a bit.

[dylan-bourque]

Thinking out loud ... we could probably use the "constant" sentinel errors and keep the current error strings with careful applications of fmt.Errorf()

Assuming ErrInvalidFormat is defined the way I mentioned before, the string returned by .Error() should be the same for the 2 expressions below

fmt.Errorf("uuid: incorrect UUID format in string %q", s)
fmt.Errorf("%w %q", ErrInvalidFormat, s)

[dylan-bourque]

LGTM. Thanks for the submission and for working with me to resolve the feedback.

[cameracker]

Hi @PatrLind , I'm finally getting some time to take a look at the open PRs. I decided to merge in the updated RFC one because it seemed like it'd have the least disruption if I merged it in first. Apologies if this generates conflicts. Also, codecov hasnt been running for a bit so the next time a change is made to this review it may come up with some missing coverage. I'll only ask for test coverage on areas you introduced.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (22c52c2) to head (ec01242).
Report is 9 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master      #131   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         5    +1     
  Lines          513       447   -66     
=========================================
- Hits           513       447   -66     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.