georgysavva / scany

Library for scanning data from a database into Go structs and more
MIT License
1.24k stars 67 forks source link

use fmt to wrap errors, not archived pkg/error #86

Closed mrkagelui closed 1 year ago

mrkagelui commented 1 year ago

currently, github.com/pkg/errors is used to return error, however since go 1.13 introduced error wrapping, this is deprecated, and the repo has been archived since.

georgysavva commented 1 year ago

Hi. Thanks for mentioning this; I didn't know that. Do you experience any particular problem with scany using the github.com/pkg/errors package? Although it's archived, I don't see any urgent reasons to replace it. But If it's something you would like to work on, I encourage you to shoot a PR with the change.

mrkagelui commented 1 year ago

firstly I'd say this error checking isn't very idiomatic after 1.13 release AFAIK.

it's more idiomatic to export the err, such as ErrNotFound, then users check it like errors.Is(err, scany.ErrNotFound), instead of the current scany.NotFound(err).

another major issue with pkg/errors especially WithStack is that, iirc, you can't call it more than 1 time. e.g., if I use this lib, and again wrap it with WithStack in my code, the previous stack info in this lib will be lost. or, if one day this lib needs to rely another lib which does WithStack there, the stack info there is lost. I haven't used it for quite long, feel free to correct me.

mrkagelui commented 1 year ago

I'll create a PR if you agree with this!

georgysavva commented 1 year ago

it's more idiomatic to export the err, such as ErrNotFound, then users check it like errors.Is(err, scany.ErrNotFound), instead of the current scany.NotFound(err).

Maybe you are right, and after 1.13 errors.Is(err, scany.ErrNotFound) makes more sense. But I don't see any problems with the scany.NotFound(err) approach either.

another major issue with pkg/errors especially WithStack is that, iirc, you can't call it more than 1 time

I think you are mistaken; it captures all stacks and keeps piling them. Then, later, you unwrap them with errors.Is() or errors.As() when checking for known errors.

All in all, I am optimistic about replacing pkg/errors with the standard implementation, but I would want not to break the current scany API.

mrkagelui commented 1 year ago

Maybe you are right, and after 1.13 errors.Is(err, scany.ErrNotFound) makes more sense. But I don't see any problems with the scany.NotFound(err) approach either.

I suppose the issue is with idiom. users new to scany wouldn't know they need this to check for a certain error

All in all, I am optimistic about replacing pkg/errors with the standard implementation, but I would want not to break the current scany API.

sure, we can have both, i.e., export the ErrNotFound, wrap them, and change the scany.NotFound(err) with checks like errors.Is(err, ErrNotFound) (with the standard lib errors) , how does that sound to you?

georgysavva commented 1 year ago

I suppose the issue is with idiom. users new to scany wouldn't know they need this to check for a certain error

We could solve it by providing the documentation. Besides that, the new users, after not founding any exported sentinel error in the library, would most likely discover the function with the corresponding name.

sure, we can have both, i.e., export the ErrNotFound, wrap them, and change the scany.NotFound(err) with checks like errors.Is(err, ErrNotFound) (with the standard lib errors) , how does that sound to you?

Sounds good!

mrkagelui commented 1 year ago

We could solve it by providing the documentation. Besides that, the new users, after not founding any exported sentinel error in the library, would most likely discover the function with the corresponding name.

Haha, exactly what I did. 😃

sure, we can have both, i.e., export the ErrNotFound, wrap them, and change the scany.NotFound(err) with checks like errors.Is(err, ErrNotFound) (with the standard lib errors) , how does that sound to you?

Sounds good!

I'll shoot a PR for a small subset of errors, if you'd find that appropriate, will do it for all.

steeling commented 1 year ago

+1 on switching to stdlib, i use a framework that check errors with errors.Is and translates to a corresponding API error. Incompatible with the current approach, need to add some hacks around it

georgysavva commented 1 year ago

Here is the new release with stdlib errors, thanks to @mrkagelui: https://github.com/georgysavva/scany/releases/tag/v1.2.0 cc: @steeling