Show choices for nullable enums in enumerable

[rcdailey]

In the non-enumerable and enumerable test cases for enumerations, I have made one of the test cases use nullable so we exercise that code path.

[adambajguz]

I think for nullable types "" should be added to choices.

This is what I have just added in Typin:

                if (Property is null)
                    return Array.Empty<string>();

                Type? underlyingType = Property.PropertyType.TryGetEnumerableUnderlyingType() ?? Property.PropertyType;
                Type? nullableType = underlyingType.TryGetNullableUnderlyingType();
                underlyingType = nullableType ?? underlyingType;

                // Enum
                if (underlyingType.IsEnum)
                {
                    if (nullableType is null)
                    {
                        return Enum.GetNames(underlyingType);
                    }

                    List<string> enumNames = Enum.GetNames(underlyingType).ToList();
                    enumNames.Add(string.Empty);

                    return enumNames;
                }

                return Array.Empty<string>();

[rcdailey]

I do not understand the purpose of having "" in the list of choices. That's not a valid choice within the enumeration itself.

[adambajguz]

Your issue is about nullable enums, so how you can input a nullable enum explicitly, e.g. when nullable enum prop has a non null default? I think the answer is "" == null.

Anyway, @Tyrrrz should comment this.

[rcdailey]

Your issue is about nullable enums, so how you can input a nullable enum explicitly, e.g. when nullable enum prop has a non null default? I think the answer is "" == null.

Anyway, @Tyrrrz should comment this.

Why would a nullable enum have a non-null default that is not one of the enumerators? I must be missing something here. Could you more clearly describe what it is you are trying to accomplish?

In my mind, a nullable enum means it is either unspecified by the user (option was not presented and its value is null by default) or it will contain a valid enumerator (a string value of the enumeration). I do not see where blank string comes in.

[adambajguz]

This is how CommandBinder is implemented. Null or whitespace input value for nullable type results in setting it to null:

           // Nullable<T>
            var nullableUnderlyingType = targetType.TryGetNullableUnderlyingType();
            if (nullableUnderlyingType is not null)
            {
                return !string.IsNullOrWhiteSpace(rawValue)
                    ? ConvertSingle(memberSchema, rawValue, nullableUnderlyingType)
                    : null;
            }

This way it is possible to set int?, double?, Enum? to null even if default is != null.

So in my mind, a null value of nullable enum means it is (from CliFx point of view):

• unspecified by user and default for property is null,
• specified by the user (default was null or not).

[rcdailey]

Is this mainly useful for positional arguments, where they must be specified in order to proceed to positional arguments after it? For example:

MyCommand.exe "" "second option"

Above we have 2 positional arguments. We set "null" for the first one (because blank string) to get to the second one? Maybe there are valid cases for this, but I would say this means the first positional argument should actually be an optional argument instead, so the user doesn't need to put anything at all.

As far as optional arguments go, why would a user want to do --my-enum-option "" instead of just leaving it out completely?

Thanks for your last explanation. I think I get the technicality of what you're trying to do, now I just need to make sense of how this helps the end user. To me, this seems to make things more semantically confusing. There is probably a scenario for this I am just not seeing.

EDIT: I want to specifically say as well that I know you are talking about options like this:

public MyEnum? MyEnumOption {get;set;} = MyEnum.SomeDefaultValue;

I think even still, I don't know why you would make a property itself optional (by not making it positional) but not have its default value 'null'. In other words, in my mind, the whole reason I made it nullable was so that it defaults to null if the user omits that option. If I wanted it to have a non-null default, I wouldn't have made it nullable.

The semantics here are VERY confusing.

[Tyrrrz]

I don't think we need "" in a list of choices. In practice, this would only take place on non-required options that accept some variation of nothing (aka null) as a valid input, but more often than not the user will just not provide the value altogether rather than set the option without a value.

I was thinking of only allowing value-less option inputs (i.e. --foo and not --foo 123) for boolean types, so that this is not a concern at all. However, the current behavior may still be useful in scripting scenarios where it's just easier to pass a value that may be empty, instead of introducing conditional logic. Overall, I think this is an imaginary problem, at least for now.

Regarding tests @rcdailey, can we separate the test cases for non-null enums from null enums? They are separate types and exercise different code paths, so it would make sense to keep them separate.

[rcdailey]

@Tyrrrz I tried to creatively consolidate what would have otherwise been 3 mostly copy & pasted test cases for each enum scenario. I haven't used Xunit before, so let me know if this is an improvement or not.

[Tyrrrz]

@rcdailey thanks! I think this change makes them a lot less readable. In general, I'm okay with code duplication in tests as long as each test can be read/understood just by looking at the method and nothing else. These are all high level tests, so they are not expected to change very often, which means code duplication is not a concern.

[rcdailey]

Made changes to the tests as requested. I'm done being clever! Hopefully this is good enough. Thank you for your patience and responsiveness to my PR!

[Tyrrrz]

No worries @rcdailey. Thanks!