Repository navigation
Run Before actions after setting up subcommand - #2028
Conversation
This fixes an issue where the Before hook of a parent command would not see flag values set in the subcommand context.
|
Actually, after thinking a bit longer, the behavior change to flag actions may not be acceptable. The existing behavior of flag actions was basically to If flag actions didn't take the However, if the context is desired in flag actions, the only option would be collecting flag instances in another slice in |
|
Having spent quite a bit of time trying to restore the behavior of flag actions while keeping the new Before logic, I can now say it's impossible. But working on that has convinced me flag actions are handled kind of incorrectly right now. At the moment, flag actions for persistent flags are invoked multiple times, basically once per 'command-level', with the value at that level. That doesn't make a lot of sense, given that the ultimate goal of flag parsing, setup etc. is to execute the innermost command action, and the flag values at that level are the ones that should be applied to the program. So I propose that flag actions of persistent flags should be run exactly once, with the value that will actually be set for the command action. The current behavior does make sense for local flags, so this can be kept. |
|
@dearchap would be nice to get your input on this PR (even though it's not fully done yet). The basic change is just to run |
|
@fjl i'm AFK and on travel. Will get to it in a day or two. Thank you so much for your patience. I have been following all the comments in this PR closely |
|
@fjl sorry for the late approval. The changes look good. |
2 participants
I'm posting this to explore a possible resolution for #2027.
Let me explain the change to
TestFlagAction/mixture. The test shows a behavior difference introduced by this PR: since flag actions will now run after all flags have been set, the test does not see the 'shadowed' flag--f_string=app. I personally think the new behavior is more correct: the flag is shadowed after all.I can't really think of a scenario in a real app where you would want to set the same flag two times with different values for the root command and the subcommand. So I propose to change the test, locking in the new behavior.
I have also added a test for issue #2027 here.
What type of PR is this?
What this PR does / why we need it:
This fixes an issue where the Before hook of a parent command would not see flag values set in the subcommand context.
Which issue(s) this PR fixes:
Fixes #2027