google / uuid

Go package for UUIDs based on RFC 4122 and DCE 1.1: Authentication and Security Services.
BSD 3-Clause "New" or "Revised" License
5.26k stars 362 forks source link

uuid.MustParse should panic error in error type #41

Closed viuts closed 1 year ago

viuts commented 5 years ago

Currently MustParse panic the error in string type,

panic should be an error type, so the recover can have consistance use of err.Error()

googlebot commented 5 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers
viuts commented 5 years ago

I signed it!

googlebot commented 5 years ago

CLAs look good, thanks!

pborman commented 5 years ago

Normally MustParse is only used when initializing global variables (or init functions). Any place else should probably be done with Parse and then check the error. I don't disagree that panicking with an error is perhaps slightly better. My only question is if it is worth enough to change the API.

viuts commented 5 years ago

@pborman I know that golang does not encourage using defer-catch to throw errors, but after suffering from all of those if err != nil, We decided to use defer-catch patterns to handle errors, especially we are using grpc. we do expect the value from recover being an error, consider the follow code.

func Catch(err *error) {
    if r := recover(); r != nil {
        panicError := r.(error)
        *err = panicError
    }
}

if the library panicking with a string, this code will get crash again when type assertion, it will be better if the library can follow the common practice which is panicking an error.

of course we can always check when type assertion

func Catch(err *error) {
    if r := recover(); r != nil {
        panicError, ok := r.(error)
        if !ok {
            panicError = fmt.Errorf("%v", r)
        }
        *err = panicError
    }
}

but it feels slightly redundant if user need to do it manually

noahdietz commented 1 year ago

Hello. I agree with @pborman that changing the value passed to panic now would be backwards incompatible to existing uses of recover catching this panic. There is no requirement that the panic be given an error, a string is allowed. So for these reasons, I will close this. Thanks for the consideration.