dotnet / command-line-api

Command line parsing, invocation, and rendering of terminal output.
https://github.com/dotnet/command-line-api/wiki
MIT License
3.39k stars 381 forks source link

best way to add option validator #1553

Open PurnaChandraPanda opened 2 years ago

PurnaChandraPanda commented 2 years ago

What is the best way to add a validator for option parse? Is "AddValidator" best here? How is to allow await statement inside validator logic that makes a db call suppose?? It works for sync but not async - maybe delegate needs to be wrapped .. could not get an example, so reaching for help.

var coption = new Option<char>("-c");
coption.FromAmong<Option<char>>("-", ".", "_");
coption.AddValidator(symbol => {
    var pvalue = symbol.GetValueOrDefault<char>();
    //await Task.Delay(2000);
    // logic for pvalue check
    return null;
});

Looking forward for suggestion!

iBicha commented 2 years ago

Expanding on this issue, I find that the symbol result is not enough to do the validation. I was hoping to access invocation context, or the an IHost so I can pull in another service that does the validation.

In my use case, validation can be done in multiple steps, by different assemblies.

jonsequitur commented 2 years ago

Our focus for validators was to validate parsing more than application logic. (I know this can be a blurry distinction and I'm not suggesting you're using it the wrong way.) For this reason, we tend to expect validation to happen based on information you already have (e.g. the command line input), and the use case for async work during validation wasn't considered in the design. Validators are run during parsing and at least right now this happens even when calculating tab completions, which is something we typically want to be as fast as possible. (Though it occurs to me we might be able to skip that for performance reasons...)

Anyway if a validator fails, it prevents your handler from being called at all, so I tend to think of it as answering the question "Is this a valid way to ask my app do something?" rather than "Is this a valid thing to ask my app to do?"

I hope that makes sense.

That said, the AddValidator API could use some work. @iBicha, your question feels related to #1549, where more context can often be useful. Are you registering the services that are performing validation in the DI container, then?

iBicha commented 2 years ago

@jonsequitur yup, things performing the validation are in the DI container

lonix1 commented 2 years ago

Did you find a way to use services inside the validator?

Here's another use case. I perform the validation in the AddValidator, and if it fails I want to print a complex error. To do that I want to use Spectre.Console which I added as one of the command's dependencies. But AddValidator doesn't expose the context, so I can't resolve that service.

Simple solution:
OptionResult should expose the context, which exposes the GetService<T>() method.

justinmchase commented 1 year ago

I cam here to file this issue also, the validator functions need to be:

For example, we have a parameter --profile. The users profiles are stored in a file. If the user specifies the profile option then that sets the profile for the single command, if they don't the default is used. One of the commands is to add a new profile, one is to switch the default profile.

We are running the parse and invoke of the command interactively in a while loop, which means we need a transient context for the service that reads the profile file on each invocation, and calculates the profile based on the default and the options. Its different for each invocation, in the same process.

That means the set of valid profiles is, for example ['A', 'B'] on the first command but if that first command is auth profile add --profile C then on the second command the set of valid profiles is ['A', 'B', 'C'].

Therefore we cannot "...expect validation to happen based on information you already have", since it changes from invocation to invocation in the same process.

However, I think this could be solved simply by altering both the .AddValidator and getDefaultValue function on the Option itself to be async and to have an InvocationContext input parameter. This would allow us to have transient state for each command and to read the valid options asynchronously from disk.

I may be missing some other use cases but it's probably fair to say that all delegates provided to the api should be async and have an InvocationContext as well.