Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nullable type #305

Closed
Jaidsuka opened this issue Jul 8, 2018 · 7 comments
Closed

Nullable type #305

Jaidsuka opened this issue Jul 8, 2018 · 7 comments
Milestone

Comments

@Jaidsuka
Copy link

Jaidsuka commented Jul 8, 2018

IsEmpty method doesn't work right in case when we use Nullable type as option. In this case when we pass 0 to parameter it won't add this option to command line back because think that default value for it will be 0 too. Nullable<'int'>.GetType() == typeof(int). IsEmpty method should be changed for covering that problem.

@moh-hassan
Copy link
Collaborator

when we pass 0 to parameter it won't add this option to command line back because think that default value for it will be 0

if you defined an option as nullable like:

       class Option
      {
        [Option('v',  HelpText = "Verbose level. Range: from 0 to 2.")]
        public int? VerboseLevel { get; set; }
     }

When 0 is passed in the command line: myapp -v 0 , the Option.VerboseLevel will be 0

For Nullable , 0 isn't the default value.

BTW, the default value of a nullable type is an instance for which the HasValue property is false and the Value property is undefined.
So, to check that VerboseLevel is assigned a value in the commandline, you can check

           VerboseLevel .HasValue // true if assihned a value in the commandline

@tydunkel
Copy link
Contributor

@moh-hassan I am running into the same issue. @Jaidsuka is pointing to the IsEmpty method in UnParserExtensions.

Using your above example, having an option defined as:

      class Option
      {
        [Option('v',  HelpText = "Verbose level. Range: from 0 to 2.")]
        public int? VerboseLevel { get; set; }
     }

Calling myapp -v 0 will set the verbose level properly, but then calling parser.FormatCommandLine(options) with those options (VerboseLevel=0) will return an empty string. This is because the IsEmpty method will return true for a Nullable when the value is 0.

This means that anyone relying on FormatCommandLine to rebuild the original commandline will not be able to do so if their options contain nullable types.

@moh-hassan
Copy link
Collaborator

PR is welcome to fix this issue.

@moh-hassan moh-hassan added this to the 2.7 milestone Sep 7, 2019
@nemars
Copy link

nemars commented Sep 15, 2019

How about something like this? This is also helping with the issue 379:
#379
#452

    private static bool IsEmpty(object value, Type type)
    {
        if (value == null) return true;
        if (ReflectionHelper.IsFSharpOptionType(type) && !FSharpOptionHelper.IsSome(value)) return true;
        if (type.IsValueType && value.Equals(type.GetDefaultValue())) return true;
        if (value is string && ((string)value).Length == 0) return true;
        if (value is IEnumerable && !((IEnumerable)value).GetEnumerator().MoveNext()) return true;
        return false;
    }

NOTICE: I removed "#if !SKIP_FSHARP" and "#endif" because this comment was being formatted as text.

@tydunkel
Copy link
Contributor

@nemars I don't think that will work. type will be a ValueType with a non-null default. This is the root of the issue. We need to determine if the type in the OptionsSpecification is actually a Nullable value type.

@nemars
Copy link

nemars commented Sep 17, 2019

That's why I put both value and type as parameters. This is called only in one place in the code and the type can easily be acquired before that call.

pi => new {
Specification = Specification.FromProperty(pi),
Value = pi.GetValue(options, null).NormalizeValue(),
PropertyValue = pi.GetValue(options, null,
pi.PropertyType)
}

@moh-hassan
Copy link
Collaborator

online demo to show the fix in v2.7+

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants