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

Added support for async programming #390

Merged
merged 7 commits into from
Feb 7, 2020

Conversation

joseangelmt
Copy link
Contributor

@joseangelmt joseangelmt commented Jan 16, 2019

With C# 7.1 you can create a pure async console application with an async Task Main() or async Task<int> Main()entry point.
The library is not easy to use in this scenario, so I added extension methods that complements all the the existing overloads of CommandLine.ParserResultExtensions.WithParsed and CommandLine.ParserResultExtensions.MapResult extension methods.

With this new async extention methods you can create async versions of sampleapps like this..

using CommandLine;
using System;
using System.Threading.Tasks;

namespace ConsoleApp1
{
    [Verb("add", HelpText = "Add file contents to the index.")]
    class AddOptions
    {
        //normal options here
    }

    [Verb("commit", HelpText = "Record changes to the repository.")]
    class CommitOptions
    {
        //normal options here
    }

    [Verb("clone", HelpText = "Clone a repository into a new directory.")]
    class CloneOptions
    {
        //normal options here
    }

    class Program
    {
        static async Task<int> Main(string[] args)
        {
            return await Parser.Default.ParseArguments<AddOptions, CommitOptions, CloneOptions>(args)
              .MapResultAsync(
                (AddOptions opts) => RunAddAndReturnExitCodeAsync(opts),
                (CommitOptions opts) => RunCommitAndReturnExitCodeAsync(opts),
                (CloneOptions opts) => RunCloneAndReturnExitCodeAsync(opts),
                errs => Task.FromResult(1));
        }

        static Task<int> RunAddAndReturnExitCodeAsync(AddOptions opts)
        {
            throw new NotImplementedException();
        }

        private static Task<int> RunCommitAndReturnExitCodeAsync(CommitOptions opts)
        {
            throw new NotImplementedException();
        }

        private static Task<int> RunCloneAndReturnExitCodeAsync(CloneOptions opts)
        {
            throw new NotImplementedException();
        }
    }
}

or like this

using CommandLine;
using System;
using System.Collections.Generic;
using System.Threading.Tasks;

namespace ConsoleApp1
{
    class Options
    {
        [Option('r', "read", Required = true, HelpText = "Input files to be processed.")]
        public IEnumerable<string> InputFiles { get; set; }

        // Omitting long name, defaults to name of property, ie "--verbose"
        [Option(Default = false, HelpText = "Prints all messages to standard output.")]
        public bool Verbose { get; set; }

        [Option("stdin", Default = false, HelpText = "Read from stdin")]
        public bool stdin { get; set; }

        [Value(0, MetaName = "offset", HelpText = "File offset.")]
        public long? Offset { get; set; }
    }

    class Program
    {
        static async Task Main(string[] args)
        {
            var parsedArguments = Parser.Default.ParseArguments<Options>(args);

            await parsedArguments.WithParsedAsync(opts => RunOptionsAndReturnExitCodeAsync(opts));
            await parsedArguments.WithNotParsedAsync((errs) => HandleParseErrorAsync(errs));
        }

        static Task<int> RunOptionsAndReturnExitCodeAsync(Options options)
        {
            return Task.FromResult(0);
        }
        static Task HandleParseErrorAsync(IEnumerable<Error> errs)
        {
            return Task.CompletedTask;
        }
    }
}

@ericnewton76
Copy link
Member

ericnewton76 commented Jan 17, 2019

TBH I feel like async is a virus that keeps infecting every method it touches. I don't know if I would've actually been happy with adding async support when we're not even doing any I/O.

The issue is, at a certain point, you have a sequence of operations that need to happen in order that can't be effectively further destructed into parts.

This opens up the possibility for the underlying library to start doing things async, however, here's my biggest opposition to this...

The commandlineparser library does not use external I/O. We don't even hit the filesystem for testing if an argument represents an existing file.

One could possibly argue that the parsing routines could be done in parallel, asynchronously, and map the results back into a single result, which would be the Options class populated with the parse results from the command line.

That all being said, it looks like a good thing to add, although right now the usage of it is kinda dubious... ;-)

@joseangelmt
Copy link
Contributor Author

It is indeed a virus that spreads until the moment when you no longer have to call any asynchronous method.

I'm already using it with the modification I've made in a process that runs on Linux micro PC that basically creates a communication server and communicates by serial port with different devices. I think there will be more contexts in which it is really necessary, and more and more.

@ericnewton76
Copy link
Member

Does this process work with the multiple verbs and MapResults extension method? (Funny thing, I actually awoke myself thinking about the MapResults method, of how to do this better) So be aware, I'm trying to figure out a better way to invoke the parser and get results or parsing errors in order to report back to the user.

I'm kinda apprehensive on this, but probably unfounded.

@ErikSchierboom
Copy link

I hit the same issue as OP.

@moh-hassan
Copy link
Collaborator

Have a look to this discussion in how to use async/await in MapResult without the need to change the Codebase of the library.

       var result = parser.ParseArguments<Options>(args);
       await result.MapResult(async 
               x =>
                     {                       
                         await new Command(x).Execute();
                     },
                 errors => Task.FromResult(0)
          );

In C# 7.1 , you can use it in Task Main().

@ForeverZer0
Copy link

TBH I feel like async is a virus that keeps infecting every method it touches. I don't know if I would've actually been happy with adding async support when we're not even doing any I/O.

I understand this reply is over two years old at this point, but Implementing async/await into the mappings is practically a necessity for this library to stay relevant with modern C#.

At the time, it may have been somewhat foreign to .NET, reserved for only niche circumstances, or just considered some syntactic sugar, but is now extremely widely used, and one of the languages most powerful features. For any application that deals with web-based technologies, it is pretty much a requirement, not an option. Having an application that pulls data from the web or database, such as a server, web scraper, etc, etc, having the the program being unresponsive during communication is simply not a possibility.

There are indeed some workarounds, but they are a bit ugly, and add extra complexity. I would really love to see this implemented as a core feature in a future release, and thank all the contributors for the fine work they have done.

@SommerEngineering
Copy link

I stumbled across this issue today. It might be that the library itself does not do I/O. However, the user code processing the options might do I/O and require async. I just had a case like that.

@joseangelmt Thank for the PR.

I will try to work around the problem in my code and continue to use async code at the same time. I hope that the PR will be accepted at some point.

@collinbarrett
Copy link

I'm brand new to using this lib, but just wondering if there are any drawbacks to using @moh-hassan 's solution? I just implemented it here and it seems to be working well.

@moh-hassan
Copy link
Collaborator

@joseangelmt
What about the use case when only one verb is async and the others are not.
For example, this code can't run and sure generate compilation errors:

    static async Task<int> Main(string[] args)
   {
    return await Parser.Default.ParseArguments<AddOptions, CommitOptions, CloneOptions>(args)
      .MapResultAsync(
        (AddOptions opts) => RunAddAndReturnExitCodeAsync(opts),  //async             
 (CommitOptions opts) => RunCommitAndReturnExitCode(opts), //not async
        (CloneOptions opts) => RunCloneAndReturnExitCode(opts), //not async
        errs => Task.FromResult(1));
}

What is the official way for mix and match async/non async parameters?
What is the return value for MapResultXXX in this case?

@antplant
Copy link

antplant commented Jul 17, 2019

@moh-hassan since the passed functions all must return TResult it's implicit that they must all return tasks in an async overload. If the consumer wants to mix and match it's trivial to wrap synchronous methods in a task with Task.FromResult, which is no different to what they'd have to do now when passing a mixture of synchronous and asynchronous methods to the existing MapResult.

That said I'm not sure that MapResultAsync really adds anything - the implementation is trivial (it can be implemented as a single line: return MapResult(result, parsedFunc, notParsedFunc);) and invoking it is no different to invoking the existing generic method, which can already be used with async functions.

@moh-hassan
Copy link
Collaborator

@antplant

it can be implemented as a single line: return MapResult(result, parsedFunc, notParsedFunc);

Yes, it can.
So, why not to implement async operations as described in this discussion without the change of the Codebase of the library.

@antplant
Copy link

antplant commented Jul 30, 2019

@moh-hassan I agree - in the case of MapResult the existing signature already gives you what you need for async as you can just use a return type of Task. There's no real need for a separate MapResultAsync overload.

The same can't be said for e.g. WithParsed, however, which forces you to use MapResult as a workaround.

WithParsed definitely benefits from an async version.

@moh-hassan moh-hassan force-pushed the develop branch 2 times, most recently from b211712 to 746885a Compare February 3, 2020 10:46
@moh-hassan
Copy link
Collaborator

@joseangelmt ,
I want to merge the PR keeping WithParsedAsync /WithNotParsedAsync and dropping MapResultAsync because the MapResult already support async/await.
Kindly, can you modify the PR and drop MapResultAsync.
It's better if you move all async methods to a separate partial file ParserResultExtensionsAsync.cs
I can do this if you agree.

@joseangelmt
Copy link
Contributor Author

@joseangelmt ,
I want to merge the PR keeping WithParsedAsync /WithNotParsedAsync and dropping MapResultAsync because the MapResult already support async/await.
Kindly, can you modify the PR and drop MapResultAsync.
It's better if you move all async methods to a separate partial file ParserResultExtensionsAsync.cs
I can do this if you agree.

Today you'll have it.

@moh-hassan moh-hassan merged commit 4d25d14 into commandlineparser:develop Feb 7, 2020
@moh-hassan
Copy link
Collaborator

Thanks @joseangelmt for the modified version of PR.
It's merged.
One note, I excluded async in Net40 with #if because it's not supported.

@rubin55
Copy link

rubin55 commented Feb 13, 2020

Quick question: how can I try this very recently merged WithParsedAsync/WithNotParsedAsync stuff? Can is simply install a newer version using dotnet add (I'm a bit now to dotnet :-) but having much fun in the process).

@moh-hassan
Copy link
Collaborator

You can use the nightly develop nuget package CommandLineParser.2.8.0-beta-125.nupkg on CI Appveyor server.

How To use:

 var result = await Parser.Default.ParseArguments<Options>(args)
                    .WithParsedAsync(RunAsync);
  result.WithNotParsed(HandleErrors); //Or WithNotParsedAsync


 async Task RunAsync(Options options)
            {
			// await ....
	   }

  Task HandleErrorsAsync(IEnumerable<Error> arg)
            {
                //handle errors
            }

@moh-hassan
Copy link
Collaborator

@ericnewton76
Can you add MyGet in Appveyor to enable reviewing knightly build packages.
Thanks.

@moh-hassan moh-hassan added this to the 2.8 milestone Feb 19, 2020
@jafin
Copy link
Contributor

jafin commented Apr 19, 2020

Can the betas/previews be published to nuget?

@moh-hassan
Copy link
Collaborator

@jafin

Can the betas/previews be published to nuget?

I'm waiting for the Nuget key update by the site Admin to publish the package.

cc / @ericnewton76
cc / @nemec

@nemec
Copy link
Contributor

nemec commented Apr 20, 2020

@moh-hassan can you send me an email to the address on my profile?

@joseangelmt joseangelmt deleted the develop branch May 3, 2020 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.