Allow passing value starting with - to custom option

[XenoPhex]

We have a case where some of our custom flags for the Cloud Foundry CLI have string values that take -1 as a value to signal special cases. For example, when we're setting a Maximum Allowed Memory Size (eg 1MB, 4GB, etc...) and we want this value to be unlimited, we use -1.

This pr will enable custom flags to take values with -.

[jessevdk]

I don't like to special case this for custom options only. If this should be supported, then I think it should be supported for all options.

The reason this is not the case, is because it leads to ambiguous parsing. In particular with flags that have an optional argument, we would need a different rule.

Therefore, the decision has been that if you do need to specify a value starting with a -, you need to use the --option=-value syntax instead. An exception was made recently for negative numbers because it makes sense intuitively to allow it (the user intent is quite clear in that case).

[luan]

@jessevdk I agree with your last point about the user intent being clear. But I think the same argument is valid on the use case @XenoPhex describes: the user there would be passing in -1 to signify that they want no memory limits, so the user intent is clear. Another way to look at this is if your custom flag type was a number with unit, for example:

distance -x1 5in -y1 -5in -x2 -10ft -y2 1ft

Or even for strings:

run-wrapper --cmd "echo" --flags "-n" --arguments "hello world"

So the argument I'm trying to make here is that allowing - as a first character in a value for a custom flag should be allowed and the onus of making sure that user intent is clear is on the custom type itself. I see that this could be misused but I say that the value of allowing this is greater than the risk of it being misused.

[XenoPhex]

any decisions on this?

[jessevdk]

Ok, your argument has convinced me. However, I would like that this works by adding a value validator for custom flags (i.e. isValidValue). The support for numbers vs not allowing - is part of the default isValidValue, but custom types can override this.

[XenoPhex]

Not really closed - I rebased incorrectly, will update shortly.

[XenoPhex]

@jessevdk Do you mean for isValidValue to be a private method (if so, on what?) or a public/interface method that any custom flag can implement?

[XenoPhex]

I've updated the pull request with my best interpretation of what you said. Please let me know if it is correct.

[jessevdk]

Sorry, what I meant was indeed an interface that a custom type may implement that can do validation of the option value. I did not mean that any custom type with a custom unmarshal will accept any kind of value.

[XenoPhex]

I'll resubmit this pull when I have some time later today.

[XenoPhex]

@jessevdk the adjustment has been submitted, please let me know if you need further changes!

[XenoPhex]

@jessevdk I've made the changes you've requested, but it looks like the Travis is failing on trying to get golint, which is weird.

I also can't seem to retrigger the build. So... hoping you can take a stick to it and get it working. Tests & lint passed locally.

[XenoPhex]

This is blocked on #283.

[XenoPhex]

@jessevdk rebased the lint changes, let me know if there's anything more I can do. Thanks!

[michaelabon]

Is this good to merge, @jessevdk?

[ewrenn8]

Hi @jessevdk, we are wondering if this PR can be merged?

Thanks,
Eli

[jessevdk]

Cool, thanks!

[XenoPhex]

Thanks for the merge!!