Adds Persistent{Pre,Post}Run hook chaining #1253

poy · 2020-10-14T04:41:39Z

PersistentPreRun and PersistentPostRun are chained together so that
each child PersistentPreRun is ran, and the PersistentPostRun are ran
in reverse order. For example:

Commands: root -> subcommand-a -> subcommand-b

root - PersistentPreRun
subcommand-a - PersistentPreRun
subcommand-b - PersistentPreRun
subcommand-b - Run
subcommand-b - PersistentPostRun
subcommand-a - PersistentPostRun
root - PersistentPostRun

fixes #252

CLAassistant · 2020-10-14T04:41:44Z

All committers have signed the CLA.

marckhouzam · 2020-10-14T10:32:18Z

Thanks @poy for the contribution.

We recently discussed this topic in #1198 and with my limited experience, I got the impression that the current behaviour allowed for the most flexibility. It allows to either override an ancestor's PersistentPreRun function by defining one in a subcmd, and it allows to extend ancestor's PersistentPreRun by calling them explicitly from a subcmd.

I'm trying to compare the value of this PR's new approach with the current one. Could you explain a little more how your change will change current behaviour, and why it is better?

I'm also curious if this change is breaking? Existing programs that have PersistentPreRuns will behave differently with this change? This was mentioned in #252 (comment).

poy · 2020-10-14T12:17:05Z

This PR will allow a user to create a hierarchy of PrePersistentRun and PostPersistentRun. Meaning if the root and sub command both have a hook, then both will be invoked. The pre-hooks get invoked in order of root -> subcommand-1 -> subcommand-2 -> ... -> subcommand-N. This allows the earlier subcommands to setup anything necessary for the laster commands. The post-hooks are invoked in the opposite order: subcommand-N -> ... -> subcommand-2 -> subcommand-1 -> root. This reverse order allows for easier cleanup (similar to how defer was implemented).

This is not a breaking change (now :) as this behavior only takes place when TraverseChildrenHooks is set to true on the root node. It will otherwise utilize the old behavior of subcommands overriding their parent's hooks.

poy · 2020-12-04T21:57:49Z

Is there any chance of this being merged?

infalmo · 2020-12-11T22:20:34Z

@marckhouzam A gentle reminder to review (and merge) this PR when free 😄

marckhouzam · 2020-12-11T23:35:08Z

@marckhouzam A gentle reminder to review (and merge) this PR when free 😄

I'm not a maintainer 😉
I just try to help when I can.

marckhouzam · 2020-12-12T16:00:18Z

This PR will allow a user to create a hierarchy of PrePersistentRun and PostPersistentRun.

@poy Could you instead call cmd.Parent().PrePersistentRun() from the subcommand?

poy · 2020-12-14T13:59:58Z

Yes. However, I think given the name "Persistent", it shouldn't be required. PersistentFlags for example don't have to be added in this way.

github-actions · 2021-02-13T00:19:54Z

This PR is being marked as stale due to a long period of inactivity

poy · 2021-06-02T03:33:43Z

Would still love for this to be considered.

PersistentPreRun and PersistentPostRun are chained together so that each child PersistentPreRun is ran, and the PersistentPostRun are ran in reverse order. For example: Commands: root -> subcommand-a -> subcommand-b root - PersistentPreRun subcommand-a - PersistentPreRun subcommand-b - PersistentPreRun subcommand-b - Run subcommand-b - PersistentPostRun subcommand-a - PersistentPostRun root - PersistentPostRun fixes spf13#252

f0m41h4u7 · 2022-12-07T12:18:18Z

Hey all.
I am also facing this issue with persistent pre- and post-runs currently, and it leads to some annoying code repeats... So I'm really looking forward to this fix being merged some day.

niamster · 2023-11-04T19:04:37Z

@marckhouzam A gentle reminder to review (and merge) this PR when free 😄

I'm not a maintainer 😉 I just try to help when I can.

@marckhouzam what is required to have this PR merged (now that you are a maintainer 😄) ?

niamster · 2023-11-04T19:40:38Z

command.go

@@ -102,6 +102,21 @@ type Command struct {
 	//   * PersistentPostRun()
 	// All functions get the same args, the arguments after the command name.
 	//
+	// When TraverseChildrenHooks is set, PersistentPreRun and


Maybe worth adding something like By default, if PersistentPreRun or PersistentPostRun is set, the parent's Pre/Post run hooks are not executed?

niamster · 2023-11-04T19:43:55Z

command.go

+	// TraverseChildrenHooks will have each subcommand's PersistentPreRun and
+	// PersistentPostRun instead of overriding. It should be set on the root
+	// command.
+	TraverseChildrenHooks bool


I think it's worth adding some checks to ensure that this is set in the root command only (because traverseChildrenHooks cares only about root command).

niamster · 2023-11-04T21:55:37Z

This PR will allow a user to create a hierarchy of PrePersistentRun and PostPersistentRun.

@poy Could you instead call cmd.Parent().PrePersistentRun() from the subcommand?

It might be very easy to forget adding that if new command creation is not automated and there are many commands added by multiple people, potentially by multiple teams.

polothy · 2023-11-06T19:31:19Z

Think this was fixed by #2044 and released in 1.8.0

niamster · 2023-11-06T22:01:48Z

Think this was fixed by #2044 and released in 1.8.0

Great news! I think it's worth closing all issues and non-merged PRs now 🎉

marckhouzam · 2023-12-18T01:09:49Z

Thank you for your efforts @niamster . I'm going to close this as #2044 addresses this.

poy force-pushed the change-persistent branch from baf1dd8 to 22954be Compare October 14, 2020 04:52

poy force-pushed the change-persistent branch 2 times, most recently from 955c56c to a51d94f Compare October 14, 2020 12:10

poy force-pushed the change-persistent branch from a51d94f to f82d6f2 Compare October 14, 2020 12:28

github-actions bot added the kind/stale label Feb 13, 2021

poy force-pushed the change-persistent branch from f82d6f2 to b528b57 Compare June 2, 2021 03:33

poy force-pushed the change-persistent branch from b528b57 to 352d20c Compare June 2, 2021 03:46

johnSchnake mentioned this pull request Feb 16, 2022

PersistentPreRun only allowed once #252

Closed

github-actions bot removed the kind/stale label Mar 30, 2022

niamster reviewed Nov 4, 2023

View reviewed changes

marckhouzam closed this Dec 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Persistent{Pre,Post}Run hook chaining #1253

Adds Persistent{Pre,Post}Run hook chaining #1253

poy commented Oct 14, 2020 •

edited

CLAassistant commented Oct 14, 2020 •

edited

marckhouzam commented Oct 14, 2020

poy commented Oct 14, 2020

poy commented Dec 4, 2020

infalmo commented Dec 11, 2020

marckhouzam commented Dec 11, 2020

marckhouzam commented Dec 12, 2020

poy commented Dec 14, 2020

github-actions bot commented Feb 13, 2021

poy commented Jun 2, 2021

f0m41h4u7 commented Dec 7, 2022

niamster commented Nov 4, 2023

niamster Nov 4, 2023

niamster Nov 4, 2023

niamster commented Nov 4, 2023

polothy commented Nov 6, 2023

niamster commented Nov 6, 2023

marckhouzam commented Dec 18, 2023

Adds Persistent{Pre,Post}Run hook chaining #1253

Adds Persistent{Pre,Post}Run hook chaining #1253

Conversation

poy commented Oct 14, 2020 • edited

CLAassistant commented Oct 14, 2020 • edited

marckhouzam commented Oct 14, 2020

poy commented Oct 14, 2020

poy commented Dec 4, 2020

infalmo commented Dec 11, 2020

marckhouzam commented Dec 11, 2020

marckhouzam commented Dec 12, 2020

poy commented Dec 14, 2020

github-actions bot commented Feb 13, 2021

poy commented Jun 2, 2021

f0m41h4u7 commented Dec 7, 2022

niamster commented Nov 4, 2023

niamster Nov 4, 2023

Choose a reason for hiding this comment

niamster Nov 4, 2023

Choose a reason for hiding this comment

niamster commented Nov 4, 2023

polothy commented Nov 6, 2023

niamster commented Nov 6, 2023

marckhouzam commented Dec 18, 2023

poy commented Oct 14, 2020 •

edited

CLAassistant commented Oct 14, 2020 •

edited