commandline icon indicating copy to clipboard operation
commandline copied to clipboard

Once any required option is processed, other required options do not generate an error if they are missing

Open NameOfTheDragon opened this issue 10 years ago • 3 comments

Here's my options class (its very simple):

public class HorizonAppOptions
    {
    [Option('s', "SourceFile")]
    public string SourceFile { get; set; }

    [Option('i', "Importer", Required = true, HelpText = "Specifies the importer to be used. Other options may be required depending on the importer chosen.")]
    public string Importer { get; set; }

    [Option('e', "Exporter", Required = true, HelpText = "Specifies the exporter to be used. Other options may be required depending on the exporter chosen.")]
    public string Exporter { get; set; }

    [Option('l', "UseLightDome", DefaultValue = false, HelpText = "Includes light dome data (if present) in the exported horizon.")]
    public bool UseLightDome { get; set; }
    }

And I parse it like this, in my Main() method:

        var caseInsensitiveParser = new Parser(with =>
            {
            with.CaseSensitive = false;
            with.IgnoreUnknownArguments = true;
            with.HelpWriter = Console.Error;
            });
        var options = caseInsensitiveParser.ParseArguments<HorizonAppOptions>(args);
        if (options.Errors.Any())
            {
            Environment.Exit(-1);
            }

When this is run, if I don't specify any options at all, I get the following output:

PS C:\Users\Tim\VS-Projects\TA.Horizon\TA.Horizon\bin\Debug> .\TA.Horizon.exe
TA 0.0.0 uncontrolled build
Copyright © 2015 Tigra Astronomy, all rights reserved

ERROR(S):
  Required option 'Importer, i' is missing.
  Required option 'Exporter, e' is missing.


  -s, --SourceFile

  -i, --Importer        Required. Specifies the importer to be used. Other
                        options may be required depending on the importer
                        chosen.

  -e, --Exporter        Required. Specifies the exporter to be used. Other
                        options may be required depending on the exporter
                        chosen.

  -l, --UseLightDome    (Default: False) Includes light dome data (if present)
                        in the exported horizon.

  --help                Display this help screen.

PS C:\Users\Tim\VS-Projects\TA.Horizon\TA.Horizon\bin\Debug>

All well and good. However, if I now specify ONE of the required options, no errors are generated and no help text is printed. Execution proceeds to the rest of my program, which later throws a contract exception because the required item is null.

PS C:\Users\Tim\VS-Projects\TA.Horizon\TA.Horizon\bin\Debug> .\TA.Horizon.exe -Exporter fluff

Unhandled Exception: System.Diagnostics.Contracts.__ContractsRuntime+ContractException: Precondition failed: !string.IsNullOrEmpty(options.Value.Importer)
   at System.Diagnostics.Contracts.__ContractsRuntime.TriggerFailure(ContractFailureKind kind, String msg, String userMessage, String conditionTxt, Excepti

It doesn't matter which required option I specify, as long as I specify one of them, the other is ignored.

I have configured the parser to ignore unknown options because I have to re-parse the options from a plugin later. However, I think it should still catch missing required options at this stage.

NameOfTheDragon avatar Mar 10 '15 18:03 NameOfTheDragon

Here is a modified unit test from InstanceBuilderTests.cs that shows how I think it should work...

    [Fact]
    public void Two_required_options_at_the_same_set_and_one_is_true() {
        // Fixture setup
        var expectedResult = new FakeOptionWithRequiredAndSet {
            FtpUrl = "str1",
            WebUrl = null
        };
        var expetedErrors = new[]
            {
            new MissingRequiredOptionError(new NameInfo("", "weburl"))
            };
        // Exercize system 
        var result = InstanceBuilder.Build(
            () => new FakeOptionWithRequiredAndSet(),
            new[] { "--ftpurl", "str1"},
            StringComparer.Ordinal,
            CultureInfo.InvariantCulture);

        // Verify outcome
        expectedResult.ShouldHave().AllProperties().EqualTo(result.Value);
        Assert.True(expetedErrors.SequenceEqual(result.Errors));
        // Teardown
    }

NameOfTheDragon avatar Mar 10 '15 19:03 NameOfTheDragon

Huh, I'm inclined to think that both examples exhibit the same behavior for different reasons.

For the unit test, you'll see that the two properties within FakeOptionWithRequiredAndSet are part of the same "SetName" which TBH is an odd name but what it means is mutually exclusive set. It's actually an error to provide both FtpUrl and WebUrl in the same argument string.

For your example, I'm not sure what the issue might be. You might try creating a new Fake based on the example and using it in place of FakeOptionWithRequiredAndSet just in case.

nemec avatar Mar 11 '15 04:03 nemec

@nemec It's trivial, really. All required options in a set actually require a value.

The way SpecificationPropertyRules.EnforceRequired() function is written, it assumes that a value per any required option per named set is sufficient.

This is a different use case (that is, likely not a bug), but I would argue that it is less obvious than the use case when all required options are checked for having a usable value.

dbakuntsev avatar Dec 09 '16 20:12 dbakuntsev