Skip to content
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

Run Before actions after setting up subcommand #2028

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

Conversation

fjl
Copy link
Contributor

@fjl fjl commented Dec 6, 2024

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?

  • bug
  • cleanup

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

This fixes an issue where the Before hook of a parent command would not see
flag values set in the subcommand context.
@fjl fjl requested a review from a team as a code owner December 6, 2024 12:44
@fjl
Copy link
Contributor Author

fjl commented Dec 6, 2024

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 invoke the action for every instance of the flag, with the value of that instance. invoke the flag at every command level. It can make sense for some applications. The problem is the context parameter.

If flag actions didn't take the context.Context parameter, they could be run before Before actions. This would make sense for applications where the flag action is used to set a global or something.

However, if the context is desired in flag actions, the only option would be collecting flag instances in another slice in Command so their local values can be accessed later when running the actions.

@fjl
Copy link
Contributor Author

fjl commented Dec 13, 2024

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.

@fjl
Copy link
Contributor Author

fjl commented Dec 13, 2024

@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 Before functions later, when the innermost values of flags have been resolved.

@dearchap
Copy link
Contributor

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Before function for persistent flags
2 participants