CLI: Refactor handling of flag forms in preparation for handling old names #10397
Conversation
tempoz
force-pushed
the
refactor-handling-of-flag-forms-for-old-name
branch
from
9e6d78c to
441627e
Compare
There was a problem hiding this comment.
I am a bit reluctant to approve this, since it's adding a relatively large amount code for a problem that may or may not be possible to fix with a smaller targeted change (it's hard for me to see where the problem is until seeing the follow-up PR that would be building on top of these changes)
If it's not too much work to explain, it might be nice to add a little more detail to the PR description as to the difficulties of handling the "old flag names" (presumably the "experimental_" stuff?) that the "Form" abstraction helps solve.
|
|
||
| const ( | ||
| // The standard form of a flag; for example: "--name" | ||
| Standard Form = iota |
There was a problem hiding this comment.
Should this be iota << 1 ? (I guess the code might be correct without this change, since the tests seem pretty exhaustive, but it seems like the intention might have been to define these as one-hot bit vectors, since you're doing some bitmasking below)
| @@ -0,0 +1,69 @@ | |||
| package flag_form | |||
|
|
|||
| type Form byte | |||
There was a problem hiding this comment.
Can you add a comment about what this represents?
One question I had also was whether Standard can only represent --name=true or if it can also represent --name=false, or if that would fall under "Negative". Might be good to clear that up here.
| UseName() | ||
| UseShortName() | ||
| UsesName() bool |
There was a problem hiding this comment.
nit: imo UseName / UsesName would be clearer as UseFullName / UsesFullName (or maybe these are better known as "long" names?)
| // Returns a formatted version of the option. | ||
| func (o *optionBase) Format() string { | ||
| switch o.Form { | ||
| case flag_form.Standard: | ||
| return "--" + o.Name() | ||
| case flag_form.Negative: | ||
| return "--" + "no" + o.Name() | ||
| case flag_form.Short: | ||
| return "-" + o.ShortName() | ||
| default: | ||
| // Just default to the standard representaation if the form is unknown. | ||
| return "--" + o.Name() | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: it's mildly confusing to me that this function doesn't return the =value part. My mental model is that --no<name> is just sugar for --name=false so it seems like we're effectively returning the flag value, but only in that one case, and not the other cases, and it seems unclear why we're doing that. This could just be a misunderstanding on my part though.
| return fmt.Errorf("Naming collision adding flag with short name %s; flag already exists with that short name.", d.ShortName()) | ||
| } | ||
| if d.HasNegative() { | ||
| if _, ok := m.ByName["no"+d.Name()]; ok { |
There was a problem hiding this comment.
darn, I guess this means we can't have a string flag called norway and boolean flag called rway :P
This PR generalizes the handling of flag forms by
Options so that we can add support for old names of flags more cleanly and easily. It also de-duplicates some formatting code, moving that functionality intooptionBase, and moves the handling of recognizing which form a flag takes from the parser and intooptionBaseas well.It does not yet have tests for
optionBase, which is why I am submitting it as a draft. When I've written tests, I will mark it ready for review.