Skip to content

feat: Add Finalize function to commands #2276

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: Add Finalize function to commands #2276

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e7e8bb0
feat: Add Finalize function to commands
Kirdock May 23, 2025
e6c2f2e
fix: Call finalize of parent and child commands
Kirdock May 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,8 @@ type Command struct {
RunE func(cmd *Command, args []string) error
// PostRun: run after the Run command.
PostRun func(cmd *Command, args []string)
// Finalize: run at the end of the Run command, even if it panics.
Finalize func(cmd *Command, args []string)
// PostRunE: PostRun but returns an error.
PostRunE func(cmd *Command, args []string) error
// PersistentPostRun: children of this command will inherit and execute after PostRun.
Expand Down Expand Up @@ -961,6 +963,7 @@ func (c *Command) execute(a []string) (err error) {
defer c.postRun()

argWoFlags := c.Flags().Args()

if c.DisableFlagParsing {
argWoFlags = a
}
Expand All @@ -981,6 +984,13 @@ func (c *Command) execute(a []string) (err error) {
parents = append(parents, p)
}
}
defer func() {
for _, p := range parents {
if p.Finalize != nil {
p.Finalize(c, argWoFlags)
}
}
}()
Comment on lines +987 to +993
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

for _, p := range parents {
if p.PersistentPreRunE != nil {
if err := p.PersistentPreRunE(c, argWoFlags); err != nil {
Expand Down
34 changes: 34 additions & 0 deletions command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2952,3 +2952,37 @@ func TestHelpFuncExecuted(t *testing.T) {

checkStringContains(t, output, helpText)
}

func TestFinalizeOfChildAndParentCalledOnPanic(t *testing.T) {
parentFinalizeCalls := 0
childFinalizeCalls := 0
defer func() {
if recover() == nil {
t.Error("The code should have panicked due to panicking run")
}
if parentFinalizeCalls != 1 {
t.Errorf("finalize() of parent command called %d times, want 1", parentFinalizeCalls)
}
if childFinalizeCalls != 1 {
t.Errorf("finalize() of child command called %d times, want 1", childFinalizeCalls)
}
}()
rootCmd := &Command{
Use: "root",
Run: emptyRun,
Finalize: func(cmd *Command, args []string) {
parentFinalizeCalls++
},
}
subCmd := &Command{
Use: "sub",
Run: func(cmd *Command, args []string) {
panic("should panic")
},
Finalize: func(cmd *Command, args []string) {
childFinalizeCalls++
},
}
rootCmd.AddCommand(subCmd)
executeCommand(rootCmd, "sub")
}