Export built-in impls of Value and corresponding new funcs

[quite]

This encourages reuse, making it possible for user to for example dynamically create a []*Flag and add them using (*Flagset) AddFlag.

Closes #334

[CLAassistant]

All committers have signed the CLA.

[tomasaschan]

While I, on the surface, understand why this is useful to export, I think we should make an effort to make sure that the API of this library doesn't grow more than necessary, and that users who want to use these features get the help they need to do it correctly.

With those points in mind, I have a few questions and requests for this change:

1. Could it be sufficient to export the constructor functions, without exporting the types? That way, we reserve the option to modify the implementation (eg if we find a way to replace some of these with a single generic one) without breaking the API.

2. The new APIs should be documented, both with docstrings and with a (possibly quite short) section in the readme that has at least one usage example.

[quite]

Yes I think this change is feasible, since API user controls the "storage container" for the value, which they pass into the "constructor" like for example pflag.NewBoolValue(optForce, &optForce). Then can then read out the value as they please. They should not need anything else.

Will have a look at it.

Tentative comment for the (bool) constructor, what do you think?

// NewBoolValue takes a pointer p which will be the storage for the
// value of a Flag. The value val is stored in p. It returns a pointer
// to a value type which implements the Value interface. This is used
// in the Value field of the Flag struct.
func NewBoolValue(val bool, p *bool) *boolValue {
    *p = val
    return (*boolValue)(p)
}

[tomasaschan]

Those docs look OK to me from the perspective of someone who knows the implementation details under the hood, but it's a little unclear to me based on just those sentences why I need to pass both a value and a pointer, especially if one will be set to the other. Does that indicate a smell in the API? 🤔

[quite]

I agree totally! It's really hard to describe to somebody uninitiated what's happening there. That val becomes the default value, right? But at the same time, the Flag struct has a DefValue string, which should be a text representation of the default value, for usage message? No really sure what's going on there.

[quite]

I think one big reason for the current API having the default value and the pointer to the storage as separate entities is because there are the BoolVar/BoolVarP/etc type of funcs that take that pointer as a parameter, and the Bool/BoolP/etc type of funcs that returns the storage pointer. Then both these types end up defining a Value, which is set into the Flag struct Value field. And its DefValue field is just "rendered" by doing value.String().

But now that we want to allow user to craft Flag struct completely on their own, we just have to export these New*Value constructors, even if they are slightly confusing, and seem to offer 2 ways to set the value (and thus also ask them to set DefValue correctly). Will have to make this more clear in the example i'll write.

Also, if we would export BoolValue (e.g.), then user would get a little help by being able to set DefValue: val.String() in the Flag struct. But perhaps that would end up exposing too much of the internals. And would not overcome the New*Value(val, ptr) anyway.

[tomasaschan]

That val becomes the default value, right?

I think so, but it's not cleat to me why, then, we don't just tell the user to set the pointer to the default value themselves prior to calling Parse?

I wonder if, instead of plainly exposing the New*Value constructors as they are, we should expose multiple constructors for each value type, corresponding to the various */Var/P variants. Not sure if that is better, but might be worth trying out on one of them to see what it looks like and if we prefer it. That has the added benefit of being similar to an API the user is already expected to understand.

[quite]

We ultimately want to allow user to craft Flag struct on their own, as much as possible. I'm guessing you mean something like the following (possibly skipping the *P variants of these, as the user can do that themselves). Looks like this could work for the bool flag type at least. Maybe it makes the setting of default value a bit more straight-forward. But the returning of both *Flag and *bool looks a bit quirky to me.

func BoolFlagVar(p *bool, value bool) *Flag {
    val := newBoolValue(value, p)
    return &Flag{
        Value:    val,
        DefValue: val.String(),
    }
}

func BoolFlag(value bool) (*Flag, *bool) {
    p := new(bool)
    val := newBoolValue(value, p)
    return &Flag{
        Value:    val,
        DefValue: val.String(),
    }, p
}

I also considered the signature BoolFlagVar(p *bool) and it doing newBoolValue(*p, p) (which seemed workable at least for the bool flag type). But then I realized that the current (for example) func (f *FlagSet) BoolVar(p *bool, name string, value bool, usage string) function really already looks and behaves like my previous proposal -- to just export NewBoolValue(val bool, *bool). User both has to pass a pointer to storage for the flag's value, and the actual value (of storage type) that shall be the default value. So I'm feeling that this plain new export is ok.

[quite]

Another attempt at documenting an exported NewBoolValue:

// NewBoolValue takes a pointer p to a bool variable, that will store
// the value of a flag, and the default value to be stored there. The
// return value is a pointer to the same variable, cast to a type
// alias that implements the pflag.Value interface. This allows it to
// be used in the Value field of a manually constructed Flag struct.
func NewBoolValue(p *bool, value bool) *boolValue {
    *p = value
    return (*boolValue)(p)
}

[tomasaschan]

Yeah, what you're saying makes sense. Thanks for indulging me and experimenting a bit! I took a deeper look at the existing function signatures and tried to come up with something that rhymed with them, but couldn't really produce anything much better than what you're proposing here, so I'm gonna say this is OK.

I like the docs you proposed in your last comment much better! Nice work!

With docs like that on all exported functions, as well as a small section in the Readme about when and how to use them, I think this is good to go!

[tomasaschan]

@quite Would having something like #359 make this better/easier? Rather than exposing a lot of new functions and types, we could expose just one generic one (which we can also build on top of for the stuff we currently have custom code for)? 🤔

[quite]

@quite Would having something like #359 make this better/easier? Rather than exposing a lot of new functions and types, we could expose just one generic one (which we can also build on top of for the stuff we currently have custom code for)? 🤔

Yes I think that could make sense. I looked very quickly at the aged PR from @mmatczuk , and from what I gather, even only implementing the generic NewValue would do the trick. Perhaps doing that, in the context of and with the same purpose as this PR and issue #334, would be a start? Let's see what they say. It's nice that pflag.Value is an interface, making this possible (given that Flag is already exported). (and very nice that pflag has zero dependencies, let's keep it like that!)