Options not set when OnValidate is called

[jerriep]

Given the following command:

[Command(Description = "List GitHub Issues")]
internal class ListIssuesCommand
{
    [Option(CommandOptionType.SingleValue, Description = "Your GitHub Personal Access token")]
    public string Token { get; set;  }

    Task<int> OnExecuteAsync(IConsole console)
    {
        console.WriteLine("This is the list of issues");

        return Task.FromResult(0);
    }

    public ValidationResult OnValidate()
    {
       return ValidationResult.Success; 
    }
}

The value for the Token property is not set when OnValidate is called, but it is set by the time OnExecuteAsync is called.

Is this by design? If so, what is the correct way to validate options in the OnValidate method?

[jerriep]

OK, what I have done for now is to access the Options on the ValidationContext, and then work with that.

My goal is to ensure that the user either passes a value via the option, or alternatively specify a value using an environment variable.

This is what I ended up with:

public ValidationResult OnValidate(ValidationContext context)
{
    if (context.ObjectInstance is CommandLineApplication application)
    {
        var tokenOption = application.Options.FirstOrDefault(o => o.LongName == "token");
        if (tokenOption != null && tokenOption.Values.Count == 0)
        {
            if (Environment.GetEnvironmentVariable(Constants.GitHubTokenEnvironmentVariable) == null)
                return new ValidationResult("You need to specify a GitHub token");
        }
    }
    
    return ValidationResult.Success;
}

[natemcmaster]

It's a bug we should fix. @atruskie noticed the same thing in #76. The fix is to change the order in which the conventions execute. At the moment, the OnValidate handler is called too soon. We need to move OptionAttribute model binding sooner.

[atruskie]

I'll be trying to solve this today in #76

[natemcmaster]

So, my current thinking is that we should add another callback that executes before line 47.

CommandLineUtils/src/CommandLineUtils/Internal/CommandLineProcessor.cs

Line 47 in 59f11a6

parseResult.ValidationResult = _currentCommand.GetValidationResult();

The conventions for OptionAttribute and ArgumentAttribute can change to use this instead of .OnParsingComplete to set property values. In terms of naming, maybe CommandLineApplication.OnArgumentsProcessed?

[atruskie]

Sounds good. I was wondering if we could just move validation to after parsing has completed?

[natemcmaster]

Yeah, that is probably better. In fact, the more I think about this, the more I realize there isn't really a good reason for the ParseResult.ValidationResult property. Seems to be mixing two things that don't belong together. I'd be okay dropping that API altogether. Users can call ParseResult.SelectedCommand.GetValidationResult() to produce the same data.

[natemcmaster]

Fixed in 2.2.1 which is now on nuget.org