Commandline: null should be supplied for missing values - not an empty string

[ohadschn]

Consider:

class MyOptions
{
    public MyOptions(string foo)
    {
    }

    [Option(Required=false)]
    public string Foo {get;}
}

If I don't specify foo on the command line, it's going to be provided as an empty string ("") in the constructor. It really should be null instead, both logically and to differentiate against an actual empty string parameter.

[ohadschn]

This is worse than I realized, even this will result in an empty string:

class MyOptions
{
    public MyOptions(string foo)
    {
    }

    [Option(Required = false, Default = null)]
    public string Foo {get;}
}

I think this is the offending line:
https://github.com/commandlineparser/commandline/blob/cd3413cc451d516a591b03760d529a251f9ea43f/src/CommandLine/Core/ReflectionExtensions.cs#L158

I could use some "secret" unique string (like a GUID) as the default to mark properties that weren't provided, but that would mess up the help text. It seems I must resort to a string wrapper class like this:

public class OptionalString
{
    private readonly string m_value;

    public OptionalString(string value)
    {
        m_value = value;
    }

    public static implicit operator string(OptionalString optionalString)
    {
        return optionalString?.m_value;
    }

    public override string ToString()
    {
        return m_value;
    }
}

[ericnewton76]

Makes sense.

I can't work on that for a while though, can you submit a pull request with the unit tests checking for null and a possible fix for it?

We're working towards a formal release of the v2 codebase into nuget very soon.

[ericnewton76]

Ironically just reading this article: https://blogs.msdn.microsoft.com/dotnet/2017/11/15/nullable-reference-types-in-csharp/

class Person //from article mentioned above
{
    [Option('f',"firstName", Default="")]
    public string FirstName;   // Not null
    [Option('m',"middleName", Default=null)]
    public string? MiddleName; // May be null
    [Option('l',"lastName", Default="")]
    public string LastName;    // Not null
}

not sure if we'll be able to determine the string? part or not, but we'll be looking into this.

Nevermind, article doesnt really address this particular nuance of null

[ohadschn]

PR submitted

[nemec]

I think this was the intended behavior by the original author (given his interest in immutability/functional style) but I'm not opposed to the change and your pull request looks good. What do you think @ericnewton76?

[ohadschn]

@nemec I'm not sure I follow, what does immutability/functional style have to do with string.Empty vs null?

[ericnewton76]

I believe the change falls in line with least surprise principal. I tried to investigate when a string.empty got substituted for null in the first place and couldnt find a logical reason why. Perhaps its related to F#?

This pull request seems fine to me. We'll see if a rash of F# guys come beating down our doors 😄 We'll point them in this direction and to explain how to somehow manage F# compliance with the existing CLS compliant languages

I watched a demo about domain driven design with F# and there was some (initially contrived) decently good reasons for using F# over others.

[nemec]

@ohadschn not to get too far into into type theory, but one common pattern in functional programming is to eliminate null entirely, often replacing it with Optional/Maybe pattern matching, which basically _forces_ the programmer to handle both the success and failure case. If you look in this library's code you'll see Optional peppered throughout.

In short, the CreateDefaultForImmutable method is used to make sure instances of types created by Commandline contain _non-null default values_ if nothing else is available - this is also why IEnumerables are initialized to empty arrays rather than null.

As they say... different strokes for different folks. I'll go ahead and merge your request since I doubt our userbase is too concerned with functional purity 😄

[ohadschn]

@nemec believe me I am the greatest null hater in the world, and I throw ArgumentNullException from just about anywhere I can. But even the SO answer you linked to doesn't talk about _eliminating_ nulls, rather making types _non-nullable by default_. I wholeheartedly support that approach, but in this case we do need null, because we want to discern between "parameter not provided" and "parameter was provided as an empty string". There is no way around it.

Optional/Maybe is exactly what the article @ericnewton76 linked to is about, and I'm super thrilled about this feature, but we need the compiler's support. It doesn't do us much good to have Maybe<T> if no one is there to stop us from doing myMaybe.Value.ToString() without checking Maybe.HasValue first (you just replace NullReferenceException with InvalidOperationException or some such). Indeed, this is exactly what we have with Nullable<T>.

[Adhara3]

In short, the CreateDefaultForImmutable method is used to make sure instances of types created by Commandline contain _non-null default values_ if nothing else is available - this is also why IEnumerables are initialized to empty arrays rather than null.

But is that intended that now IEnumerables behaves in a different way than string? Isn't that misleading? And why the default is not honored even when explicit?
If I write:

[Option("custom-app-priority", Default = null, Required = false, Separator = ',']
public IEnumerable<string> CustomAppPriority { get; set; }

I probably really do want a null. And getting an empty array when I asked for a null is very error prone and unintuitive.

Don't you agree?
A

[moh-hassan]

@Adhara3

But is that intended that now IEnumerables behaves in a different way than string?

From the Framework Design Guidelines 2nd Edition (pg. 256):

DO NOT return null values from collection properties or from methods returning collections. Return an empty collection or an empty array instead.

Empty IEnumerable reduce Null References(NRE): The Billion Dollar Mistake :) and reduce null check.

[Adhara3]

The guidelines are perfect, but this scenario is a bit different, IMHO.
The user here is explicitly specifying a default value and his will is not respected. I specifically ask for null, I get empty array.
If you want to stick with guidelines then Default = null should not be valid and should generate a compile time error or something similar.
If the user does not specify any default , then you can use whatever the guidelines suggest, but if the user can override the default then it's no more a guideline problem, it's a user will problem.
Moreover, empty array may be a meaningful value for my application which may not be the default value for that parameter. How do I distinguish then a non provided value from a provided empty value?

Thanks
A

[moh-hassan]

Null or empty for IEnumerable properties can be checked using one method.

This change can cause a Breaking Change in the application using the library but it may be applied in V3.

Default = null should not be valid and should generate a compile time error or something similar.

Or it may be ignored(but can be documented in wiki).
What is your suggestion?

Edit:
All value type ( int, long ... ) can not be null, even if Default= null.

[vesuvian]

Google landed me here because I'm running into the same Enumerable null default problem as Adhara3.

My two cents: I think null is descriptive in saying "this option was not used" rather than "the user provided no parameters for this option". Checking for null is totally safe, whereas checking for .Any() could show warnings about multiple enumeration.

Providing a Default parameter and then not obeying that is really unhelpful. It would be helpful if an exception was thrown for null because it at least clarifies the intended behaviour, but I'd prefer if null was handled properly and an exception only be thrown if the property type is non-nullable.