Switch to using async all the way

[rahulpnath]

• Update all method to be Async
• Updated the Function Names to reflect Async
  This will force to update the code and make sure to add await in the consumer code
• Returning AsyncEnumerables for the paged requests endpoints
• Currently not supporting the sync endpoints
  Should it be?

#55

[mroloux]

Thanks, that's quite a PR 😆

Could you explain to us non-experts what the advantage is of switching to async calls? And whether it makes sense to support both sync and async calls?

[rahulpnath]

Thank you for the quick response @mroloux . I'll try my best to explain it in simple terms and also put in some recommended links/articles 😀

Async/Await is the way to perform asynchronous programming in .NET.

When synchronously calling the HTTP methods you are essentially blocking the calling thread until you get back a response from the other side. Whereas with using asynchronous programming, as soon as you submit your HTTP request, the calling thread is free to do other work, until you get back a response from the other side.

This is a great reference on introduction to Asynchronous Programming in .NET

Support both sync and async? I don't think there is a need to do this. Given that you are already using RestSharp the Execute method is just a wrapper around the async call as you can see here

    public static RestResponse Execute(this IRestClient client, RestRequest request, Method httpMethod)
        => AsyncHelpers.RunSync(() => client.ExecuteAsync(request, httpMethod));

You can also read more about it here. Do you see a need to support sync versions as well?

If required by any of the consumers, they can use the same AsyncHelpers as used in the RestSharp code above.

Let me know if that help.

[mroloux]

Alright. Let's go with the async version then.

I would use the original method names though, e.g. Create instead of CreateAsync.

Also, should we add an optional CancellationToken token parameter to each method?

[rahulpnath]

@mroloux Great! Adding in a cancellation token sounds excellent. I will update the PR.

As for keeping the same Function name, this might cause issues for anyone using it today unless they make sure to add the await keyword. Having the function name different will force them to look at all the places explicitly and change to the XXXAsync version and have them think to add in the await as well.

But this could be something that can be called out in the release doc/Readme. Happy to go with whatever way you decide to go ahead with!

[mroloux]

Yeah, it's a tradeoff. But I'm afraid we'll never get rid of the XXXAsync naming if we don't bite the bullet now.

The release notes will need to be very clear about this of course.

[rahulpnath]

@mroloux Having the Async suffix is a general convention in .NET. Here is a guideline from Microsoft.

Add "Async" as the suffix of every async method name you write.

It is even more helpful in this case since it's a breaking change.

That said, I will leave it to you to finalize before I work on the PR again next week. Have a good weekend 😀

[mroloux]

Ok, let's keep the XXXAsync names then if it's a convention 👍

[mroloux]

One small remark: it would be nice if ListChartsTest.MultiplePages() would still assert that the order of retrieved charts is correct.

This assertion should work:

Assert.Equal(charts.Select(c => c.Id).OrderByDescending(x => x), retrievedCharts.Select(c => c.Id));

[rahulpnath]

@mroloux Added in cancellation token support and also ensured that ListChartsTest.MultiplePages() checks the order.