Expose parsed options object in ParserResult

[shravan2x]

I tried using this library in my project today. I'm able to parse the command line from the examples just fine, but found no way to access the parsed options object directly from ParserResult.

By my coding style, I prefer not use callbacks here (i.e. WithParsed and WithNotParsed). I'm already able to use the Tag property to check if parsing succeeded. Am I just missing something here or can a property exposing the parsed object be added?

[moh-hassan]

You can access Options from ParserResult :

   var options =  (result as Parsed<Options>)?.Value;

[shravan2x]

@moh-hassan That's good to know, I'll use that. I think it would still be nice to have a property available neatly. Are there any specific challenges preventing that?

[johnjaylward]

having a simple option to get the parsed options value back would be great. Currently with the callback options, it makes it very difficult to use the Main return value, or to use an asynchonous Main.

// doesn't work...
public static async Task<int> Main(params string[] args)
{
    Parser.Default.ParseArguments<Options>(args)
        .WithParsed(await RunMyActualProg)
       .WithNotParsed(ShowSomeError);
    // how to get my return value from `RunMyActualProg` without some weird hacks?
}

// works but is ugly...
public static async Task<int> Main(params string[] args)
{
    Options options = null;
    Parser.Default.ParseArguments<Options>(args)
        .WithParsed(o => options = o;)
       .WithNotParsed(ShowSomeError);

    if ( options !=null )
    {
       return await RunMyActualProg(options);
    }
    return 15; // some error code. Arg issues should have been printed from above
}

It would be nice to just do something like this instead:

// much cleaner than above
public static async Task<int> Main(params string[] args)
{
    
    (Options options, IEnumerable<CommandLine.Error> errors) result =
        Parser.Default.ParseArguments<Options>(args).Result;

    if (result.options != null)
    {
       return await RunMyActualProg(result.options);
    }
    if (result.errors != null)
    {
        foreach (var err in result.errors) { /* do something with errors */ }
        return 1; // command line error
    }
    return 15; // some error code. Arg issues should have been printed from above
}

I think having 3 properties available directly on the ParserResult would be great:

• T Options { get; } which just returns the options, or default if parsing failed
• IReadOnlyCollection<Error> Errors { get; } which should never return null. I'd prefer IReadOnlyCollection so a Count check can be performed easily
• (T, IReadOnlyCollection<Error>) Result { get; } which just returns a ValueTuple of the 2 properties above

I think this makes the API much more flexible and readable to people who don't, or can't use the callbacks to run the application.

I could live without the 3rd option, and just store off the ParserResult using that directly as well.

// much cleaner than above
public static async Task<int> Main(params string[] args)
{
    
    ParserResult<Options> result = Parser.Default.ParseArguments<Options>(args);

    if (result.Options != null)
    {
       return await RunMyActualProg(result.Options);
    }
    if (result.Errors?.Count > 0)
    {
        foreach (var err in result.Errors) { /* do something with errors */ }
        return 1; // command line error
    }
    return 15; // some error code. Arg issues should have been printed from above
}

[moh-hassan]

@johnjaylward
V2.8 include new WithParsedAsync / WithNotParsedAsync to support async/ await and can be used inside async static Task Main

[johnjaylward]

The new methods don't really solve the issue here

[moh-hassan]

Can you provide the use case that is not working by example.

[johnjaylward]

From the original above:

By my coding style, I prefer not use callbacks here (i.e. WithParsed and WithNotParsed). I'm already able to use the Tag property to check if parsing succeeded. Am I just missing something here or can a property exposing the parsed object be added?

The main issue was not having to use the WithParsed and WithNotParsed methods at all.

The Async was just a concrete example of how those methods didn't help.

It also still doesn't solve the issue with return values from int Main(params string[] args).

Can we just move the Value property from the Parsed<T> object and put it on ParserResult<T> instead? It seems like a more logical place to put it since ParserResult is already generic however it makes no public references to the generic type.

[moh-hassan]

If you need to return value to Main, use MapResult

[johnjaylward]

No offense, but I find using the WithParsed and WithNotParsed to be very awkward. I don't really see the point of having a command-line parser being part of my call stack which makes looking at debug logs and stack traces awkward. Once the command line is parsed and I have my Options back, I have no need for the parser object to stay in the stack in any form. It's preferable to me that I get my options in a single step and then let the parser be garbage collected.

Something simple like this is optimal for me from a command line parsing perspective:

public static async Task<int> Main(params string[] args)
{
    
    Options options = Parser.Default.ParseArguments<Options>(args)?.Value;

    if (options == null)
    {
        return 15; // some error code. Arg issues should have been printed from above
    }
   
    return await RunMyActualProg(options);
}

This to me shows a clear separation of concerns (command line parsing vs running the program) and I don't have random LINQ type statements making tracing my program take longer than needed.

Having WithParsed and MapResult calls just adds unneeded overhead, both from a call stack as well as from developer review.

[moh-hassan]

Good casting to get option.
What about in case Verbs?
Both WithParsed and MapResult are Extension methods for ParserResult and you can add your own.
Separate concerns are respected and both methods execute method /function or lambda expression and auto cast Option class especially you have many verbs.

[johnjaylward]

Good casting to get option.

I don't agree that that's a good way. It requires the developer to know the implementation that is returned. If an update to the library changes which implementation of the base class is used, the cast will return null. It would be a lower barrier to entry for new users of the library to have a simple property on the base class to access the parsed value or the error array. If the implementation classes don't have a valid value for those properties, returning a decent default value or null would be good.

Would you consider a PR for this if I put one together?

What about in case Verbs?

Not needed for simple cases where a developer just wants to parse some simple options and use them right away.

Both WithParsed and MapResult are Extension methods for ParserResult and you can add your own.

That puts an onus on a consumer of the library to do something that should be fairly out-of-the-box

Separate concerns are respected and both methods execute method /function or lambda expression and auto cast Option class especially you have many verbs.

I would disagree. The With* methods keep the parser in the stack for the call. This is probably fine for more complex user-cases like Verbs (like you mention), but when someone just wants to capture a -v and some/path/to/file.txt it muddies the executing code with the parser which I feel is unnecessary.

[moh-hassan]

ParserResult is an abstract class and option is accessed via Value property and remove parser from stack.
Parser deal with simple and complex many verb cases.
Can you provide an alternative solution ?

[sipsorcery]

If you need to return value to Main, use MapResult

@moh-hassan I've ended up coming back to this issue 2 or 3 times when I add CommandLineParser to a new project and have forgotten how to use the ParseArguments result.

When you say use MapResult do you mean something like this?

var parseResult = Parser.Default.ParseArguments<Options>(args);
Options opts;
List<Error> errors = null;
int res = parseResult.MapResult(o => { opts = o; return 0; }, e => { errors = e.ToList(); return 0; });

if(errors?.Count > 0)
{
  Console.WriteLine($"Parsing failed: {String.Join('\n', errors)}");
}

[johnjaylward]

@sipsorcery , I believe that pattern should work.

The one I've been using is to just ignore the errors as the default parser generally prints them to stderr anyway. It looks like this:

Options? options = (Parser.Default.ParseArguments<Options>(args) as Parsed<Options>)?.Value;
if (options == null)
{
    // usage is automatically shown if options == null, which will be true if there is ANY problem parsing the args
    return 2; // or whatever your args-parse error code is.
}
// my options got parsed. run my program

As long as I don't need to log the argument errors somewhere, I find this is the simplest form to use (at least until v2.9 gets released).

[johnjaylward]

This is also a decent way if you do want the errors:

ParserResult<Options> parserResult = Parser.Default.ParseArguments<Options>(args);
if (parserResult is NotParsed<Options> errors)
// also works by checking the `Tag` property parserResult.Tag==ParserResultType.NotParsed
{
    // do something with errors
}
else if (parserResult is Parsed<Options> opts)
// also works by checking the `Tag` property parserResult.Tag==ParserResultType.Parsed
{
    // do something with opts
}
else
{
    //somehow the result was null or something else completely unexpected
}