-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Let the commands store flagComp functions internally (and avoid global state) #2012
base: main
Are you sure you want to change the base?
Conversation
Thanks @maxlandon for trying to improve things. However, this is actually how the original implementation tried to do it in #1423 which broke flag registration as described in #1437 and fixed in #1438 |
Thanks for pointing those to me. |
@marckhouzam thanks for the quick reply in any case, I see that you guys are very busy on this repo. I am also sorry for unearthing this thing, and doubly sorry to insist on the need for this to be solved out a way or another. I will open a thread dedicated to this, to expose more things in detail about that. In any case, I would be very, very, very grateful if it was possible to gather a little bit of attention once again on the present issue. Sincerely, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach work for most cases but it will not work for persistent flags right now.
If you have a persistent flag defined on the root cmd you do not get that completion on a child command. In order to support that you would need to walk the tree up to root command when you lookup the completion function.
True, I completely forgot this initialization problem. I will look at it this weekend, but normally I just need to add initialization in the related functions. |
Normally the initialization problem should be fixed with the last commit I pushed. Not sure how to solve for persistent root command' flags, but working on this. |
Hello @marckhouzam, sorry to ping but I think this is very urgent: So in this PR and in the last 3-4 commits, the following changes/fixes have been made:
You would be very, very kind to review and/or test this as quick as possible, so that we can avoid too many people using the global flag comp getters in their code. Related to #2019 Ping me ruthlessly if needed for anything, at any time. |
I missed this. Sorry. |
@maxlandon This does not compile |
@marckhouzam i'm going to fix this later today and will ping back |
@maxlandon I have added extra tests in #2053 to protect us against regressions. |
Thanks a lot @marckhouzam , could you compile the code in this PR ? Or do you still need a fix ? Away from my laptop today unfortunately, but can be available to help for the rest of the week and beyond. Edit: I can take care of merging the various branches on which tests have been added, as well any other for that matter. Edit 2: I can probably do the whole thing in a new PR, with everything gathered/fixed/documented in one place. |
I got it to compile (see small diff at bottom [*]), but I get a panic running the tests.
Just rebase your PR on top of #2053 as two commits.
No need for a new PR. Please just squash all your changes into one commit and force-push. [*] patch to fix compilation:
|
@maxlandon Can you clarify (I still don't understand, sorry), why we need to avoid global state.
Ok, that make sense, but it is not dramatic, I think.
This is already how it is done. A developer registers the flag completion right next to the flag declaration. They don't know how the registration is stored in cobra.
I don't get this one. Can you give an example.
I can't grok that sentence. Do you want to be able to do this, or are you trying to avoid it? |
@marckhouzam below is the main, summed up reason for this, followed by the 3 points aiming to illustrate /clarify your last reply. Main reason: in short, having global state (for completions or else) is in effect making an assumption about the lifecycle of commands and their completions. This assumption is impractical, useless and risky, while not being improving clarity of code related to our present matter. 1) Garbage collection (ex: closed loop applications) 2) Flags compFuncs binding close to command declaration // Current (global state)
cmd := cobra.Command{}
cobra.RegisterFlagCompletion(myFunc)
// Without global state, directly through the command method.
cmd := cobra.Command{}
cmd.RegisterFlagCompletion(myFunc) From above you can see that:
3) Parent/child commands and their compFuncs Consider this: // root command has a flag and its completion.
parent := cobra.Command{}
parent.RegisterFlagCompletion("myflag", flagFuncParent)
// Child command has other flags and completions
child := cobra.Command{}
child.RegisterFlagCompletion("childflag", childCompFunc)
// No let's query the compfuncs
parentComp := child.GetFlagComp("myflag") // recursively looks up to the parent until found
childComp := child.GetFlagComp("childflag") // immediately found
notFoundComp := parent.GetFlagComp("childflag") // not working since lookup is always up the tree. This is conforming with how cobra looks up the command trees when devising what to complete. 4) the sentence you can't grok |
I did not see this message (i'm on my phone atm). Will do as indicated ! |
That makes sense.
Actually, your second example is how things are done today, but the
I believe we can already do this using the new |
Hello @marckhouzam , I was actually working on this at this moment ! So to "answer your answer": More commits to solve all of this in a few minutes/hours ! |
Actually, to answer your third point ( Global function, querying the global map thanks to a flag pointer: Line 149 in 890302a
A command method, querying the global map thanks to a flag name: Line 158 in 890302a
So with the changes in the current PR, the global function |
Okay normally this should be good. Summarizing the work done and steps taken below:
Will pinpoint these numbers in the commits code, if you want to have visual confirmation for all of them. |
completions.go
Outdated
func GetFlagCompletion(flag *pflag.Flag) (func(cmd *Command, args []string, toComplete string) ([]string, ShellCompDirective), bool) { | ||
flagCompletionMutex.RLock() | ||
defer flagCompletionMutex.RUnlock() | ||
func (c *Command) GetFlagCompletion(flag *pflag.Flag) (func(cmd *Command, args []string, toComplete string) ([]string, ShellCompDirective), bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change this to a command method.
completions.go
Outdated
completionFunc, exists := flagCompletionFunctions[flag] | ||
return completionFunc, exists | ||
// Or walk up the command tree. | ||
return c.Parent().GetFlagCompletion(flag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Recursively walk up the command parents' tree until flagComp is found.
Behavior identical regardless of if the flagComp is searched with a flag pointer, or a flag name.
completions.go
Outdated
flagCompletionMutex.RLock() | ||
completionFn = flagCompletionFunctions[flag] | ||
flagCompletionMutex.RUnlock() | ||
completionFn, _ = finalCmd.GetFlagCompletion(flag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Use our new recursive methods to lookup what to complete when
__complete
is called.
Passes all tests
Thanks for the update @maxlandon ! As a release might be cut in the next hours, I prefer to play it safe and start with #2063 to avoid rushing your PR (which is more complicated and requires a more detailed review, as you saw from the repeated required rework.) |
Doing the review on the other PRs at the moment. I would still pretend that it is better to review/accept/reject this one before any release. |
@marckhouzam could you just allow the workflows to run once on this one ? just to make sure I'm not defending something that doesn't pass the bar :) ! EDIT: Nevermind, just saw that you ran them 15 minutes ago ! |
Thanks for the ping: I'd be happy to help get this through. Looking now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks solid. I'm also going to spend some time testing this against kubectl
and it's completions to ensure we're not missing / breaking anything
@maxlandon can you rebase please |
This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
@marckhouzam (and @jpmcb for that matter here) Even better than a rebase, I renamed Now everything is solved, logic is good for both functions. Hopeful to see this go through ! I appreciate the attention you all have given to this thing recently ! Thanks a lot ! |
Hello @marckhouzam, any chances to see this merged soon ? |
Problem
Currently, flag completion functions are a global map, preventing garbage collection
to work properly, especially if the commands using these funcs are garbage-collected themselves.
This might happen if commands are declared, bound and used through yielder functions.
This is also produce more coherent API usage in different cases:
In addition, and while in theory nothing prevents a given command
cmd1
to be called withRegisterFlagsCompletion(cmd2Flag)
, and still store and work correctly with another commandflag.
Changes
Add the old
flagCompletionFunctions
map and its mutex as unexported fields of theCommand
.