georgysavva / scany

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

chore: replace archived pkg/error with std lib errors #90

Closed mrkagelui closed 1 year ago

mrkagelui commented 1 year ago

fixes #86

mrkagelui commented 1 year ago

@georgysavva kindly let me know your opinion.

you might feel a bit uncomfortable for removing "WithStack", but I think go idiom treats errors as values, and adding context with fmt.Errorf when necessary is the recommended way, like what pkg/errors provided as errors.Wrap.

if you have time, perhaps https://go.dev/blog/go1.13-errors helps

georgysavva commented 1 year ago

Thanks for the PR. It looks good; I agree with everything except for using "%v". I think it should always be "%w" when propagating errors from the underlying database lib.

mrkagelui commented 1 year ago

Thanks for the PR. It looks good; I agree with everything except for using "%v". I think it should always be "%w" when propagating errors from the underlying database lib.

ok cool, I'll wrap with %w and expand it to everything

mrkagelui commented 1 year ago

would you like to approve running workflows, or would you want to wait until I finish everything?

codecov[bot] commented 1 year ago

Codecov Report

Merging #90 (1f78d17) into master (fe4b088) will decrease coverage by 0.11%. The diff coverage is 61.90%.

:exclamation: Current head 1f78d17 differs from pull request most recent head 7ec9b52. Consider uploading reports for the commit 7ec9b52 to get more accurate results

@@            Coverage Diff             @@
##           master      #90      +/-   ##
==========================================
- Coverage   87.13%   87.02%   -0.11%     
==========================================
  Files           5        5              
  Lines         342      339       -3     
==========================================
- Hits          298      295       -3     
  Misses         33       33              
  Partials       11       11              
Flag Coverage Δ
unittests 87.02% <61.90%> (-0.11%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
dbscan/dbscan.go 81.42% <61.90%> (-0.48%) :arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

mrkagelui commented 1 year ago

sorry for the wait @georgysavva, kindly take a look

mrkagelui commented 1 year ago

Hi @georgysavva is there any update?

georgysavva commented 1 year ago

Hi @mrkagelui. Everything looks great; thanks for your work!

The only thing that concerns me: the go.sum file should also have changed after you updated go.mod. So you probably need to run go mod tidy to actualize the go.sumfile and commit the changes. When it's done, I will merge the PR.

mrkagelui commented 1 year ago

Hi @mrkagelui. Everything looks great; thanks for your work!

The only thing that concerns me: the go.sum file should also have changed after you updated go.mod. So you probably need to run go mod tidy to actualize the go.sumfile and commit the changes. When it's done, I will merge the PR.

Sure thing!

mrkagelui commented 1 year ago

Hi @mrkagelui. Everything looks great; thanks for your work!

The only thing that concerns me: the go.sum file should also have changed after you updated go.mod. So you probably need to run go mod tidy to actualize the go.sumfile and commit the changes. When it's done, I will merge the PR.

Hi, I've actually done those, I'm afraid that's because it's a transitive dependency. go mod why github.com/pkg/errors gives

➜  scany git:(chore-use-std-error) ✗ go mod why github.com/pkg/errors
# github.com/pkg/errors
github.com/georgysavva/scany/dbscan
github.com/georgysavva/scany/dbscan.test
github.com/cockroachdb/cockroach-go/v2/testserver
github.com/cockroachdb/cockroach-go/v2/testserver/version
github.com/pkg/errors

check https://github.com/cockroachdb/cockroach-go/blob/master/go.mod for a reference (I think they need such a PR too 😆 )

mrkagelui commented 1 year ago

Hi @georgysavva just in case you missed this

georgysavva commented 1 year ago

Hi @mrkagelui. I am sorry for the slow correspondence and thank you for pinging me!

I just checked the go mod. I agree with you about the go.sum file. So I am going to merge the PR and release a new version including this change. Thank you so much for your work!