Only split environment variables on commas when explicitly told to do so?

[deanishe]

I ran into an issue yesterday when a string flag's value was being truncated, and I eventually figured out that it was ff splitting the environment variable on the comma, leaving the flag set to only the last few characters of the input.

I believe that splitting by default is not a good idea because none of the default flag types actually support multiple values, meaning the default behaviour is effectively to (incorrectly) truncate input if there's a comma in it.

Splitting is also currently an all-or-nothing proposition: you can have automatic splitting on if you've defined your own custom slice-based Value, but then you also run the risk of standard flag.String values being borked if their input contains a comma.

I think a better solution would be for environment variable splitting to be explicitly opt-in for specific flags or specific types (if that's possible), e.g. ff.WithEnvVarSplit("flag1", "flag2", ...) or ff.WithEnvVarSplit(*CustomValue1, *CustomValue2, ...).

[peterbourgon]

That's fair, although it's a breaking change, meaning the major version would have to be bumped.

[peterbourgon]

Fixed in v3.

[deanishe]

I've looked at the changes. v3 just switches the default behaviour to not splitting, right?

If the delimiter appears in a flag that shouldn't be split (e.g. a flag.String), the value will still be truncated, won't it?

[peterbourgon]

The default behavior is not splitting, and you can opt-in to splitting on a custom delimiter.

ff doesn't distinguish between "flags that shouldn't be split" and "flags that should be split" because that distinction is arbitrary. The flag.Value interface has a Set method, which can always be called multiple times. What the concrete implementation chooses to do with multiple Set calls isn't the concern of package ff — or package flag!

That is, it's valid to define a flag.String "s", and start your program with

./myprogram -s one -s two -s three

The value of that flag will be "three", and this isn't wrong or a bug or anything. If the flag happened to be a type that implemented Set by appending to a slice, or inserting to a map, the result would be different. EnvVarSplit is a way to get similar behavior if you want to specify flags with env vars. That's all.

Does this answer the question?

[deanishe]

I appreciate that. It's not the handling of flags that is the issue, but of environment variables. That wasn't explicit in my last comment. I wrongly assumed it would be clear from the context of the issue.

Consider a program that accepts one API key and multiple tags (i.e. -tag is assigned to a custom flag.Value that collects a []string, while -apikey is assigned to a flag.String).

This works:

./myprogram -tag blue -tag yellow -tag orange -apikey yUTEMLg,+Aj
# tags   = []{"blue", "yellow", "orange"}
# apikey = "yUTEMLg,+Aj"

This doesn't:

export TAG=blue,yellow,orange
export APIKEY=yUTEMLg,+Aj
./myprogram
# tags   = []{"blue", "yellow", "orange"}
# apikey = "+Aj"
#
# - or -
#
# tags   = []{"blue,yellow,orange"}
# apikey = "yUTEMLg,+Aj"

With splitting turned on, it's basically equivalent to running ./myprogram [...] -apikey yUTEMLg -apikey +Aj

If the user runs that command themselves, fair enough, that's user error. But they haven't done anything wrong, so it's a bug in the program.

I can fix it by replacing any usage of flag.String with a custom flag.Value or doing the splitting manually, but that's something every ff user will have to do if they want to split some environment variables without others getting erroneously truncated.

As such, I believe a better solution is what I suggested above: ff should only split environment variables for flags it's explicitly told to split and leave the rest alone.

[peterbourgon]

Use a split delimiter that isn't a valid character in your API key.

[deanishe]

It doesn't just apply to API keys, does it?

"Be sure to use a delimiter that can't possibly appear in any of your string inputs" is quite the restriction.

[peterbourgon]

I don't think it is.

I appreciate your position, but this is already an incredibly esoteric feature, I don't think adding another option to the API to make its behavior even more esoteric is going to be net positive for the package.

[deanishe]

but this is already an incredibly esoteric feature

I'm sorry. I don't really see how not being able to split some environment variables on commas and also have commas be allowed in other variables without truncating them is "incredibly esoteric".

I don't think adding another option to the API

I don't think you should add another option. I think the ff.WithEnvVarSplit() function should take one or more flag names instead of a delimiter.

is going to be net positive for the package

Fair enough. Your package, your call.

[cbandy]

This is long closed, but my suggestion would be for two flags: tags and tag. One would split on commas while the other would not. https://play.golang.org/p/_khKLDSprsW

./myprogram -tag blue -tag yellow -tag orange -apikey yUTEMLg,+Aj
# tags   = []{"blue", "yellow", "orange"}
# apikey = "yUTEMLg,+Aj"

./myprogram -tag blue -tags yellow,orange -apikey yUTEMLg,+Aj
# tags   = []{"blue", "yellow", "orange"}
# apikey = "yUTEMLg,+Aj"

export TAGS=blue,yellow,orange
export APIKEY=yUTEMLg,+Aj
./myprogram
# tags   = []{"blue", "yellow", "orange"}
# apikey = "yUTEMLg,+Aj"