Comments (10)
Its not a bug when its the programmer's own mistake for misusing the function..... Adding a panic comes with huge risk of breaking your app, if anything you should use static code analysis like linter instead to prevent the misuse.
If you add panic it will not be detected anyway before the code is comitted, you would have to wait until it is deployed to production -> wait until panic occured -> then fix it, which is very costly imo
from stacktrace.
+1 -- I don't like the behavior of passing a nil
error causing the entire function to return nil
. To me, it's a form of nil
hiding that adds cognitive overhead -- would much prefer to be able to say that, whenever a stacktrace.Propagate
call exists, we expect and error to exist.
from stacktrace.
Weak disagreement.
I would note that the superior errors package supports the return nil
behavior. I don't have strong feelings on this once we switch to pkg/errors, because we no longer need to wrap every error return with a propagate/wrap function call, turning the bulk of the returns where we use this feature into return foo()
or return bar, err
, making this a moot point.
As icing on top, we can ditch our stacktrace parser that extracts the cause messages (see deployctl's nostacktrace).
from stacktrace.
OK, after thinking about this a bit more, I realize that my initial language was not precise enough/didn't fully capture my intent.
I actually do agree that propagate
or wrap
should return nil
when provided with nil
-- I think this makes the most sense from an API standpoint. You can't really return an error because the entire point of the call is to produce an error (so you can't distinguish intended output from failure), and you definitely don't want to panic.
Instead, my contention is that making use of this behavior as client code is bad style because it obfuscates intention/behavior -- someone needs to understand the API/look under the covers to see what it's doing, and it's confusing because you see the error message right next to the call, so you have a tendency to think that an error is occurring.
To me, this is unambiguous:
if err := doSomething(); err != nil {
return {propagate|wrap}(err, "message")
}
return nil
And while the following is more compact, it takes more cognitive overhead for me to understand that, if the function doesn't error, the rest of the call is basically a no-op:
return {propagate|wrap}(doSomething(), "message")
It may just be a matter of getting used to it, but that's my view.
from stacktrace.
On the macro point, I'm definitely interested in trying out pkg/errors
-- I like that you don't have to wrap at every level and still get a lot of the benefit. What would it take to migrate a larger project like Skylab? Is it just a matter of ensuring that all top-level calls that deal with errors use the %+v
formatting directive?
from stacktrace.
RE: what would it take:
- use
%+v
formatting directive - replace
stacktrace.Propagate
witherrors.Wrap
andstacktrace.NewError
witherrors.New
- for functions that use
stacktrace.PropagateWithCode
,stacktrace.NewErrorWithCode
andstacktrace.GetCode
, define functions likeos.IsExist(error)
- update deployctl to
Println(err.Error())
and delete our nostacktrace code
from stacktrace.
I still disagree with your argument that the propagate/wrap call with possibly nil error is bad style.
Let's observe how error handling looks like in idiomatic go:
https://golang.org/src/encoding/json/encode.go?h=return+err#L824
822 buf, err := tm.MarshalText()
823 w.s = string(buf)
824 return err
This is common throughout the standard library, and I'd argue the most sensible way to attach context to this error is:
822 buf, err := tm.MarshalText()
823 w.s = string(buf)
824 return errors.Wrap(err, "resolve %v", w)
I think it makes it unnecessarily verbose to handle the err != nil
case explicitly. The reviewer had to check that it made sense for us to return err == nil
in both the original and the wrapped code. Nothing has changed here. Just because we're passing a value to a function does not mean you get to assume the value is correct.
it obfuscates intention/behavior -- someone needs to understand the API/look under the covers to see what it's doing
Isn't the obsfucation correct here? typeFields
does not care whether or not tm.MarshalText()
failed: it will return its error return value either way here. If the underlying MarshalText
gets changed, then the behavior of typeField
changes as well.
from stacktrace.
Caught up with Nick offline. I'm warming up to the idea of only wrapping non-nil errors. The example that convinced me is fmt.Errorf
. It's the standard library's equivalent of errors.Wrap
, and it is never used on a nil error. I'm okay with proceeding with pkg/errors
and writing new code to check the error before wrapping.
from stacktrace.
I absolutely love this library and use it in all our Go code, but Propagate allowing nil
has been a huge pain point for us. Very commonly what happens is something like:
myVal, found := myMap[theKey]
if !found {
return stacktrace.Propagate(err, "No value found")
}
stacktrace.Propagate
was accidentally used here in place of stacktrace.NewError
, but rather than failing loudly it becomes a difficult-to-hunt-down issue of trying to find out why your function is mysteriously succeeding when it shouldn't. Making Propagate panic when it receives a nil error would be a huge timesaver for us; for scope, we've probably burned at least 10 hours on bugs of this sort.
EDIT: An alternative idea would be making a nil
input to stacktrace.Propagate
behaving like stacktrace.NewError
; that would help with the fail-loudly idea
from stacktrace.
For anyone in the future: we hit yet another bug where Propagate
was incorrectly used instead of NewError
, this repo hasn't had any commits in 6 years, and it's very likely that a large amount of Palantir's codebase is dependent on the current behaviour (meaning this issue likely won't ever close).
We therefore forked this repo and made Propagate panic when its cause is nil
: https://github.com/kurtosis-tech/stacktrace , for anyone who wants a fix for this.
from stacktrace.
Related Issues (6)
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from stacktrace.