Unintuitive ValueObject behavior

[kyle-fritz-zocdoc]

ValueObject, the object that is returned from Docopt.Apply() has some unintuitive behavior in its type checking properties. This class is a wrapper around an object that was parsed from the user args. Helpfully, the ValueObject contract has several properties that allow one to check the type of the argument token: IsList, IsString, IsInt.

My main issue is with the IsInt property. As far as I can tell, it is never true, even when integer arguments are supplied. That's because the IsInt property is implemented by doing a type check with the is keyword on the object with which the ValueObject is constructed. However, the ValueObject is constructed with the string-typed token. While something like that might work in python, it will fail consistently in .NET. This results in a weird situation where objects that have IsInt = false have proper values for their AsInt properties.

There's a couple ways one could fix this. Changing the IsInt property to return the result of a call to Int32.TryParse() would work. Alternatively, the code that instantiates ValueObject could attempt to convert the string prior to invoking the constructor.

I may be off-base here if the IsInt property was never meant to be used be consumers of this library. If that is the case and this property is behaving in this way purposely, may I ask that we make it internal instead of public?

[voieducode]

IsInt should really be named IsOfTypeInt because I really mean it to test for type compatibility (straight python code translation) rather than to confirm that a value can be potentially treated as an integer.

So how about implementing IsInt with Int32.TryParse() the way you describe and redirect the current internal usages of IsInt to a IsOfTypeInt method? I wonder if AsInt should not be revisited as well?

If IsInt was directly implemented using Int32.TryParse() the test LanguageAgnosticTests.Test_104 would be broken because "--speed=20" would be wrongly be considered of type int.

Regarding the constructor, ValueObject is known to take arguments of type int in the context of incremented flags as produced by:

Usage: prog [-vv]

See CountMultipleFlagsTests.

[kyle-fritz-zocdoc]

Okay. I can propose a PR with something like that. But is there any value to having IsOfTypeInt exposed to consumers of the library? Your description of it makes it sound like it is used only internally. If that's so, I'd make it internal.

Relatedly, there is an IsString property. This property is implemented like IsOfTypeString. If we create IsInt with this implementation, should it be permissible that something that has IsInt = true also have IsString = true? On the one hand, it can be used like a string. On the other hand, one would think these properties would be mutually exclusive. I can see a good argument for both sides.

[voieducode]

Agreed, internal it should be. Sounds to me like IsOfTypeInt and IsOfTypeString should be mutually exclusive. Have a look at the Match method in LeafPattern.—
Sent from Mailbox for iPad

On Wed, Dec 18, 2013 at 6:02 PM, Kyle Peter Fritz
notifications@github.com wrote:

Okay. I can propose a PR with something like that. But is there any value to having IsOfTypeInt exposed to consumers of the library? Your description of it makes it sound like it is used only internally. If that's so, I'd make it internal.

Relatedly, there is an IsString property. This property is implemented like IsOfTypeString. If we create IsInt with this implementation, should it be permissible that something that has IsInt = true also have IsString = true? On the one hand, it can be used like a string. On the other hand, one would think these properties would be mutually exclusive. I can see a good argument for both sides.

Reply to this email directly or view it on GitHub:
#5 (comment)

[kyle-fritz-zocdoc]

Yeah, clearly IsOfTypeInt and IsOfTypeString should be mutually exclusive. My question was regarding the behavior of the new public properties IsInt and IsString. Should they behave like "CanBeConvertedIntoInt" and "CanBeConverteredIntoString", in which case a integer would be true for both? Or should they behave mutually exclusively where an integer value is not considered a string?

[voieducode]

Thanks for the PR @kyle-fritz-zocdoc!

I think the first interpretation (CanBeConvertedIntoInt) makes sense as a general scenario and it would be consistent with properties like StringExtensions.IsInt. I do not see a scenario where a client needs to know if the value is stored as an int.

I will push a release with your changes soon.

[voieducode]

Fix released as part of 0.6.1.5.