Introduce a thin error API with optional stack trace capturing. #429
Conversation
Codecov Report
@@ Coverage Diff @@
## main #429 +/- ##
==========================================
- Coverage 68.99% 68.93% -0.06%
==========================================
Files 94 95 +1
Lines 9706 9726 +20
==========================================
+ Hits 6697 6705 +8
- Misses 2289 2301 +12
Partials 720 720
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks @jhchabran ! This is a pre-review, I have a few questions before continuing the review 👍🏼
| @@ -261,7 +262,7 @@ func scanDocument(iter document.Iterator) (types.Document, error) { | |||
| } | |||
|
|
|||
| if d == nil { | |||
| return nil, errs.ErrDocumentNotFound | |||
| return nil, errors.New(errs.ErrDocumentNotFound) | |||
There was a problem hiding this comment.
Should we change how we create errs.ErrDocumentNotFound instead?
// in errors/errors.go
import "github.com/genjidb/genji/internal/errors"
var ErrDocumentNotFound = errors.New("document not found")There was a problem hiding this comment.
It's already done as you suggest, but because how it is declared, the stack trace is meaningless, so this is why there is another .New call that corrects that.
I believe that we'd better address those in a different PR, so we can take a step back on how we want to handle the errors, and keep this one simpler.
| @@ -507,10 +507,10 @@ func (p Path) IsEqual(other Path) bool { | |||
| // GetValueFromDocument returns the value at path p from d. | |||
| func (p Path) GetValueFromDocument(d types.Document) (types.Value, error) { | |||
| if len(p) == 0 { | |||
| return nil, ErrFieldNotFound | |||
| return nil, errors.New(ErrFieldNotFound) | |||
| // it returns nil, enabling to wrap functions that returns an error directly. | ||
| // If a string is passed, a new error is created based on that string. | ||
| // If any other type is passed, it will panic. | ||
| func New(e interface{}) error { |
There was a problem hiding this comment.
If think this function should be called Wrap instead of New:
- In both build modes, it doesn't actually create a new error in all cases.
- It mixes creating an error out of a string and returning / wrapping an existing error.
- Its counterpart is named
Unwrap - When it's used with an existing an error, it feels weird to read
errors.New(e)
This is a handy function but it mixes two needs that could be split in more explicit functions:
- wrapping an error
func Wrap(e error) error {
if e == nil { return nil }
// no-debug
return stringutil.Errorf("%w", e)
// debug
// stacktrace et autres joyeusetés...
}- creating a function out of a string:
func New(e string) error {
// no-debug
return errors.New(e)
// debug
return Wrap(errors.New(e))
}There was a problem hiding this comment.
I strongly hesitated on this one, though I entirely agree that New(error) feels a bit weird albeit it's quite handy. I'll change it.
| } | ||
|
|
||
| // Is performs a simple value comparison between err and original (==). | ||
| func Is(err, original error) bool { |
There was a problem hiding this comment.
In non-debug, non-wasm mode, we are using fmt, reflect etc.
Should we also use the std errors.Is as well?
There was a problem hiding this comment.
This conversation made me doubt so I just did a few tests, just to be sure: with Tiny Go v0.18, it now has support for reflect and thus works with the std errors package. This changes thing, I believe.
v0.18 👇
This release is the combination of more than two months of work, including improvements to reflection (...)
|
|
||
| // Unwrap does nothing and just returns err. | ||
| // This function only acts differently when the debug version of this function is used. | ||
| func Unwrap(err error) error { |
There was a problem hiding this comment.
Same here for the std errors.Unwrap
|
Closing this in favour of #431. |
The goal of this package is to enable to capture stack traces when the error are being raised, which speeds up the development process, if enabled at compile time. To do so, this set of changes introduces an
internal/errorspackage to handle errors with four functions:.New,.Errorf,.Isand.Unwrap.If the code is compiled with the
debugbuild tag, the stack trace will be captured when an error is created or wrapped (both with.New). Without that build tag, theerrorspackages falls back to simply comparing values and performs no wrapping at all.This means that it's usable for targeting WASM because it does not depends on
reflect/reflectliteas opposed to the standarderrorspackage when.Isor.Asis called (such a good call @asdine to point me out that issue before I hit the wall 🙏) There's obviously no free lunch, it also means that we lose along the way the ability to wrap errors and thus be able to get the original type (1).It also come with a new
testutil/assertpackage which replaces therequirepackage when it comes to checking or comparing errors and printing the stack traces if needed.Finally, the
testtarget of the Makefile uses the debug build tag by default. Atestnodebugtarget is also provided for convenience and to make sure no tests breaks due to not having used theinternal/errorsortestutil/assertpackage.For the above, the diff is rather small and located in
internal/errorsandtestutil/assert. The rest, the massive set of changes is merely a grind through the codebase to update the API. I have double checked myself on those.I'm sure there must be a few errors unwrapped, it's hard to catch everything, but. that should cover most of it.
(1) There was one occurence of this, in the online doc package I recently added, but I removed it along the way because it didn't bring much.
PS: please don't mind the commit names, we'll squash as usual :)