Conversation
Add go.mod, targets to build, test and lint. Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
crazy-max
left a comment
There was a problem hiding this comment.
Needs to bump actions to latest stable following Node 20 EOL coming soon on GitHub Hosted runners
3b5125c to
c321b9f
Compare
| "error\n" + | ||
| "github.com/pkg/errors.TestFormatNew\n" + | ||
| "\t.+/github.com/pkg/errors/format_test.go:26", | ||
| "\t.+/format_test.go:26", |
There was a problem hiding this comment.
Curious about these; were the tests wrong, or did behavior change between Go versions? (ISTR something changed in some of stdlib).
If it changed in Go, was that before, or after go1.20 (otherwise tests won't be valid with the minimum version specified in go.mod)
There was a problem hiding this comment.
These tests were pre-gomod and expected checkout to be in GOPATH that is not a requirement anymore.
| //go:build go1.13 | ||
| // +build go1.13 | ||
|
|
||
| package errors |
There was a problem hiding this comment.
I would squash the removal of the tags with the previous commits, but keep the removal of the Makefile in this one.
There was a problem hiding this comment.
These are opinionated changes, while the first commit is purely making the linter and test pass with the recent version of Go without changing anything about the code. Moved the makefile away.
| //go:build !go1.26 | ||
|
|
||
| package errors |
There was a problem hiding this comment.
This commit starts the work of picking up maintenance; I think it would make sense to move this (and forward) separate.
(Steps before this were just setting the baseline)
| // GlobalE is an exported global to store the result of benchmark results, | ||
| // preventing the compiler from optimising the benchmark functions away. | ||
| var GlobalE interface{} | ||
| var GlobalE any |
There was a problem hiding this comment.
any was added in go1.18, and the module already had go1.20 as minimum; I think we can squash this one with the "fix linting" commit, or move it before the addition of the go1.26 features.
There was a problem hiding this comment.
modernize is not same as lint. These changes make the code use the latest conventions but are not required for the code to work properly and lint pass in recent Go.
| branches: | ||
| - master |
There was a problem hiding this comment.
Perhaps add main to the list, so that we can rename the default branch.
This commit could also be put before the go1.26 feature commit, making it part of the "baseline".
There was a problem hiding this comment.
Renaming branches does not seem compliant with maintenance mode.
|
Left some comments (sorry I reviewed per commit, so some got superseded in later commits). Overall looks good; some things I think we should do;
|
|
I’m not sure the stacktrace feature is even useful. Most people tend to declare their errors at the top level, where the stacktrace ends up being |
|
@puellanivis There is no obligation to use this package. This package works well with typed/static/chained errors; you add the stack trace to the return path, not to the definition path, if you use static errors. |
Fix gofmt formatting, simplify code patterns flagged by gocritic/staticcheck/revive, update test path expectations to not assume GOPATH layout. Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Remove go1.13 build constraints from unwrap functions since go.mod minimum is go1.20. Rename go113.go and go113_test.go to unwrap.go and unwrap_test.go. Delete Makefile as build/test/lint are now handled by Docker Bake targets. Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Remove defunct Travis-CI and AppVeyor badges. Replace the outdated Roadmap section with a comparison to the standard library's errors package. Update Contributing section to reflect current maintenance-mode policy. Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
puellanivis
left a comment
There was a problem hiding this comment.
Yes, that was the going advice to people at the time: that global errors should use stderrors since this package’s stacktraces are really only useful when they appear at the return.
My point of raising this point was that use of this package has always been with significant special caveats, and footguns. It was just that the calculus worked out in favor of using it, since one could wrap and unwrap errors.
Now, the whole utility of this package is really just the WithStack function, and I’m not really sure that the same calculus holds anymore. It might be better to just propose and implement a feature request on the standard errors package and add stack tracing to that.
It’s basically: there’s a reason why this package fell into an unmaintained state, and it was not really for lack of care or interest to maintain it.
| // as a value that satisfies error. | ||
| // Errorf also records the stack trace at the point it was called. | ||
| func Errorf(format string, args ...interface{}) error { | ||
| func Errorf(format string, args ...any) error { |
There was a problem hiding this comment.
🤔 With the now standard library Errorf using the %w verb, this function is now subtly different from how likely a large number of people are people are using fmt.Errorf. This creates a situation where we’re likely to create significant surprise.
| } | ||
| } | ||
|
|
||
| func TestJoin(t *testing.T) { |
There was a problem hiding this comment.
Since our Join is just return stderrors.Join(errs...), testing this is perhaps not particularly appropriate? It is, after all, not really our responsibility; it’s the standard library that has written the tests.
| } | ||
| } | ||
|
|
||
| func TestAsTypeNotFound(t *testing.T) { |
There was a problem hiding this comment.
Unlike TestAsType above, and more like the TestJoin, we’re basically testing either stderrors.As or stderrors.AsType… facing the same question of whose responsibility that testing belongs with, us, or stderrors.
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Maintainers from @moby project can take over keeping this repo available in maintenance mode.
From README:
@thaJeztah