feat: Add Finalize function to commands #2276
Conversation
This adds a "Finalize" function to the command, which is executed at the end of every run, even if the run panics
| defer func() { | ||
| for _, p := range parents { | ||
| if p.Finalize != nil { | ||
| p.Finalize(c, argWoFlags) | ||
| } | ||
| } | ||
| }() |
There was a problem hiding this comment.
I have a doubt with panics and recover.
You said this will be called even the command panics.
But I see no recover in the code block that could panic.
Is the recover somewhere else I didn't see ?
The recover you added in the test might make it works.
Did you try with a real command that panics and some finalize with fmt.Println debug.
Also shouldn't the finalizer be aware it panicked?
Is there a state somewhere in the command ? Or something you could pass to the finalizers? But maybe, it would change its signature
There was a problem hiding this comment.
You said this will be called even the command panics.
But I see no recover in the code block that could panic.
and
Did you try with a real command that panics and some finalize with fmt.Println debug.
A deferred function will always be called, no matter if the function panics or recover() is called.
Are you referring to the fact that finalizers could also panic? At the moment, I've kept it the same as for the global finalizers, but if one finalizer panics, the rest of the finalizers won't be executed, yes.
But I see no recover in the code block that could panic.
So, would you assume, if the "Finalize" is set, that the command should not panic? Then we would also need to touch the global finalizers.
Also shouldn't the finalizer be aware it panicked?
I just thought we should use the same implementation as for the global finalizers. I would see it as a finally, like in try-catch of other languages. We can still call recover before executing the finalize functions and forward the info func(cmd *Command, args []string, panicked bool, reason any) or just func(cmd *Command, args []string, panicReason any). How do you want to proceed?
There was a problem hiding this comment.
Thanks for the detailed reply.
You are right. I was misunderstanding (I had forgotten) the way defer works with a panic.
I would say that for now your PR is good, because:
- you replicated something that exists
- you seem to understand the stack better than me 😅
Let's wait for a maintainer feedback, such as Marc.
For now, I don't think there is a need to change your PR
This adds a "Finalize" function to the command, which is executed at the end of every run, even if the run panics.
Done as a follow-up for #2275