-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Fix LocalFlags().NFlags to count the number of local flags that have been set explicitly #1999
base: main
Are you sure you want to change the base?
Conversation
lnpflags stores localNonPersitentFlags
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. |
@johnSchnake |
Because the problem is a bit more complex, I have detailed the modifications. It is long, but it would be thankful if you would read it.
Issue
This issue is addressed in #1315.
LocalFlags().NFlag
andLocalNonPersistentFlags().NFlag
are provided to count the number of local flags that have been set explicitly, but they do not work as expected.LocalFlags().NFlag()
always return 0.Cause of the issue
NFlag
method is defined as follows,From the definition of
NFlag
, it looks like LocalFlags().actual is not set.f.actual
is set in theSet
method.This
Set
method is called inside the processing ofc.Flags().Parse(args)
.c.flags
andc.lflags
are different objects, and Parse method is performed on only c.flags (not on c.lflags). That's why c.lflags remain empty andLocalFlags().NFlag()
return 0.How I solved
1. execute Parse for lflags
As I mentioned above, the cause of issue is that Parse method is not performed on lflags. So I
change it to perform Parse method on lflags.
https://github.com/spf13/cobra/pull/1999/files#diff-4d7d1923693fc5ce892add2ea2907a744e77ea0b50c1939ccc5067cb48a466a3R1885
2. fix
addToLocal
logic in LocalFlags functionPerforming Parse method on lflags is not enough to set lflags.actual.
As I showed above, actual field value is set only if
flag.Changed == false
in the Set method. The flags in c.lflags are pointers and therefore have the same value as the flags in c.flags. In other words, at the time of LocalFlags().Parse(), flag.Changed is true, so exit Set method before setting the value in lflags.actual. Therefore, I assign false to f.Changed before adding it to c.lflags.https://github.com/spf13/cobra/pull/1999/files#diff-4d7d1923693fc5ce892add2ea2907a744e77ea0b50c1939ccc5067cb48a466a3R1635
3. remove parent
Flags passed as arguments to lflags.Parse() must be limited to flags registered in c.lflags. Therefore, I added a process to exclude flags other than those in c.lflags from the passed arguments.
https://github.com/spf13/cobra/pull/1999/files#diff-4d7d1923693fc5ce892add2ea2907a744e77ea0b50c1939ccc5067cb48a466a3R1767-R1855
The above modifications to LocalFlags() are also made to LocalNonPersistentFlags().
Discussion
Setting f.Changed to false before adding flags to c.lflags may cause problems. This is because setting f.Changed back to true again assumes that the Parse method will be executed immediately afterwards.
I thought about creating a new flag object instead of using the flags in c.flags so that c.flags would not be affected, but some of completion unit tests did not pass.
If you have any good ideas, I'd love to hear them.
Related Issue