Skip to content

Add async API for a few extension methods #474

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

Closed
wants to merge 2 commits into from

Conversation

IEvangelist
Copy link

This will enable task-based Main entry point in C# 7.1+ projects -- which would be awesome.

@moh-hassan
Copy link
Collaborator

Thanks @IEvangelist for your work.
What is the drawback of using the library in async/await as described in this discussion without the change of the CodeBase of the library.
What is the difference with this PR #390 that also support async/await.

@IEvangelist
Copy link
Author

I like what I see in #390, merge that one... mine is less complete.

And as far of the discussion you call attention to, well I suppose you could do that. It is correctly accepting a Func<T, TResult> -- so that could work but honestly, it's a bit ugly. I'd prefer the first class citizen overrides that are purely Task based. Thanks, feel free to close this one and please consider merging #390. 👍

@moh-hassan
Copy link
Collaborator

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 errors:

        static async Task<int> Main1(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));
    }

We should use ALL func(s) as async or ALL not async.

@IEvangelist
Copy link
Author

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 errors:
static async Task Main1(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));
}

We should use ALL func(s) as async or ALL not async.

That is why my suggestion in this pull request wasn't to use .MapResultAsync, instead it added the appropriate overloads so that the consumer could pick and choose / mix and match.

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

    await result .WithParsedAsync(async options => { ... await  } );
    result.WithNotParsed(errors => { ... });    
}

They could easily live together if not explicitly bound with the .MapResult* functions. Those methods introduce cohesion between the success and error states, but I was proposing overloads for the individual .With* calls which are nicely independent. Make sense?

@moh-hassan
Copy link
Collaborator

moh-hassan commented Jul 16, 2019

I think that I can't pick and choose / mix and match. (I mean PR #390)
I tried and get a compilation error:

warning CS1998: This async method lacks 'await' operators and
will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
error CS0029: Cannot implicitly convert type 'int' to 'System.Threading.Tasks.Task'
error CS1662: Cannot convert lambda expression to intended delegate type because some of the return types in the block are not implicitly convertible to the delegate return type

You can try this example using the dev package of asyn of PR #390 here .

@IEvangelist
Copy link
Author

IEvangelist commented Jul 16, 2019

I'm not sure what your code looks like, but it works. I just verified it locally... Here is the screen capture of a unit test within the commandline project itself, this exemplifies what I tried calling attention to above.

image

@moh-hassan
Copy link
Collaborator

That is good for the method WithParsedAsync with only one verb,
I mean multi verbs: one verb is async and the other verbs are not async.
I tried the example in the PR 390 for MapResult with three verbs and try to mix and match but get a compilation error.

All the parameters of the extension methods of PR #390 are using:

              Func<Tn, Task<TResult>>  parsedFuncn

kindly, can you give me other example for MapResultAsync with the the three verbs: one async and the others are not async. Maybe I missed something.

@IEvangelist
Copy link
Author

That is good for the method WithParsedAsync with only one verb,
I mean multi verbs: one verb is async and the other verbs are not async.
I tried the example in the PR 390 for MapResult with three verbs and try to mix and match but get a compilation error.
All the parameters of the extension methods of PR #390 are using:
Func<Tn, Task> parsedFuncn

kindly, can you give me other example for MapResultAsync with the the three verbs: one async and the others are not async. Maybe I missed something.

I didn't write the .MapResultAsync, I only wrote the .WithParsedAsync and .WithNotParsedAsync overloads. But, regardless of the number of verbs -- the method overloads that I wrote will work with mix and match...

image

@moh-hassan
Copy link
Collaborator

I didn't write the .MapResultAsync, I only wrote the .WithParsedAsync and .WithNotParsedAsync overloads. But, regardless of the number of verbs -- the method overloads that I wrote will work with mix and match...

Yes, you are correct. I discussed PR #390 because you have an opinion for merging it.
I'll discuss the issue of MapResult with @joseangelmt and feedback the result.
Thanks for your co-operation. :)

@IEvangelist
Copy link
Author

That makes more sense. I guess I was all for the merging of #390 because it added the async APIs that I selfishly cared about, i.e.; the .WithParsedAsync. 😄

@moh-hassan moh-hassan force-pushed the master branch 2 times, most recently from 5e09c7d to 182e72f Compare March 13, 2020 20:43
@IEvangelist IEvangelist deleted the add-async branch May 12, 2020 18:52
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.

2 participants