-
Notifications
You must be signed in to change notification settings - Fork 17.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
proposal: errors/errd: helper package for deferred error handlers #32676
Comments
|
I don't think Apply carries its weight. (It seems helpful here largely to describe how Add and Wrap work.) The example code isn't quite right; I think you meant:
But if I were going to do that, I would instead write
That is clearer and faster: it only need refer to the single If you had a more complicated function to apply, it seems fine to just make it take a
The version without Apply isn't obviously better here, as above, but it's not clearly worse either. |
This is probably too bike-shed-y at this early phase, but I wonder if there is a better name than Trace since the concept of "trace" has an unrelated antecedent in the Go library and runtime. Perhaps:
|
Following two sample codes missing err in last argument.
|
Thanks @rsc, this looks great to me. Personally this addresses the only remaining concern I had with the main |
Whether or not try() is going to be implemented, this errd package is a great idea, because, I have to admit, that after thinking about it, using defer for error handlers makes sense in many cases, as long as performance is not critical. Unlike @cespare I think the One use case which seems to be missing is logging the error using the log package. I think and errd.Log() function could be very useful. However, I think there is one way that would make this even more useful and that would be to make it a "fluid" API, a bit like this: package errd // import "errors/errd"
type errd * error
func New(err * error) errd {
return errd(err)
}
func (e errd) Trace() errd
func (e errd) Log() errd
func (e errd) Logf(fmt string, args...interface{}) errd
func (e errd) Apply( f func(err error) error) errd
func (e errd) Add(errp *error, format string, args ...interface{}) errd
func (e errd) Wrap(errp *error, format string, args ...interface{}) errd Then we can easily compose error handlers like this: |
What is the rationale for the name? |
I personally like the idea of |
Simple question as I am obviously missing something. Why a pointer to Also, does |
Why do you feel the need for a separate package, rather than putting these in I don't see why
Shouldn't the two formatting functions end in I've seen the name |
I made a quick hack and the results are interesting: https://gitlab.com/beoran/errd So now I can handle an error with where closer has a Close method like this: The potential is limitless, there could even be .If(), .Switch(), so the error handling could get as complex as I would like it to be. But I guess not everyone likes method chaining, so maybe it's better that this remains an alternate package, much like how we have std lib log and logrus. |
@jba, I would expect everything in errors to be about errors and not about error pointers. Putting the deferrables in their own package lets them have a different name space. I think that solves one of the big naming problems we had with clunkers like fmt.HandleErrorf. |
@cespare, I agree that Apply doesn't carry its weight for func literals. It might if people write other (named) helper functions. |
@johanbrandhorst, Yes, errd is short for err-defer. If these are going to be used a lot, it makes sense for them to have a short name (like fmt.Printf not format.Printf). And if the package were errors/deferred then the uses would look like:
which is a bit repetitive. |
It only helps insofar is it allows such helpers have the signature
So even then it seems like a wash. I'd rather establish the convention that custom error helpers have the signature |
The name |
Of course, that stutters ( |
I read the name as "erred", as in to err --- someone erred, now it's time to deal with consequences. |
I am generally in favor of this proposal, but share concerns about the name. That said, it's not an easy package to name! I'm wondering about defer handle.Trace(&err)
defer handle.Add(&err, "copy %s %s", src, dst)
defer handle.Wrap(&err, "copy %s %s", src, dst) |
I see several people dislike my experiment. I have a feeling why this might be so, although I'd prefer to hear why. After all, if the idea of doing error handing using |
I'm actually fond of fluent APIs myself, but I don't think this one is necessary. In the relatively rare cases where you need multiple error handlers, you can just write multiple Another problem is that the only way to add another action is to modify your package. |
Thanks! I'd like to see this as part of errors. |
This would also make easier to add some simpler way to defer error handlers at a later stage. For example, try may have the signature (in pseudo-code): func try(expr, handlerFunc, args ...) (T1, T2, … Tn) with the added benefit that it would not be needed to name return values. I think it is obvious enough how this would work (and this issue is not the right place to discuss it) but I think this is a nice example of why a more consistent definition is desirable, instead of having the choice between |
@beoran as someone who does not have the spare time to ramp up 100% on this new error handling stuff, the current error handling proposals are frankly, terrifying to me from a complexity standpoint if the above statement is accurate. Note that my perspective coming from that of a new user looking at this for the first time, maybe second time. I would be very surprised if other people did not share this opinion under the premise that they did not want to aimlessly re-iterate this concern, especially without concrete feedback and reasoning for their claims. I thought your experiment was useful, even though I did not agree with it, because it was able to provide a discussion point of what could happen once other users started to adopt this idiom. Food for thought: my initial reaction to this is simply the question of how this is better than exception handling as a whole. In addition to looking very complex, it also looks unfamiliar. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
thanks for your reminder. |
This package (or another) could also add a handler for closing, like Close(errp *error, io.Closer) to do proper error handling when closing a writer or even to solve the "panic problem" for CloseWithError. The point is, error handling like this may be a bit odd, but it allows us to solve a few subtle gotchas in an idiomatic and straightforward way. |
@mpvl, yes I already added a |
As someone pointed out on another issue, d, err := someFn()
try(errors.Wrap(err, "some msg")) To support this use-case, maybe these functions should return an error value. |
I'll chime in with greatly preferring func() (err error) {
defer errd.Apply(&err, func(err error) error {
// this check will apply to errs from both fn1, and fn2's potentially-applied-err,
// and any new code that appears below it in the future
if err == ... {
err = ...
}
})
_, err = try(fn1())
defer errd.Apply(&err, func(err error) error {
// currently only applies to fn2's err
if err == ... {
err = ...
}
})
_, err = try(fn2())
} It seems much safer and easier to me to pass the same fn to |
In the Go 2 Error Values proposal (#29934), one of the most common complaints I saw was the lack of an explicit |
I was hoping it also returned the error so you can use it in a plain return also?
|
This should probably be split into two distinct packages
The part of My biggest concern is that |
What is the fate of this proposal now that #32437 has been declined? It is of course still possible to use |
Given that |
Update 2: Update: I just shared a comment about this issue on another project, as that project has started adopting some of the code-snippets Here is the link to the question I posted mentioning this issue: |
There is no errors.Wrap in stdlib, only .Unwrap. |
Okay, thank would explain it! Thank you, @networkimprov |
The try proposal (#32437) uses a placeholder
fmt.HandleErrorf
to annotate errors.We do not intend that to be the actual function name. It was a blank to be filled in.
Here is one possible way to fill in that blank.
Thoughts and improvements welcome.
/cc @mpvl @jba @neild
The text was updated successfully, but these errors were encountered: