I'm really not keen to do this, because everything in the Octokit API (ignoring Octokit.Reactive which just wraps it) returns a Task. And I'll point to the documentation the NServiceBus crew put out explaining why they don't do this either (yeah, they also refer to this project, whatever).

But I asked @terrajobst if this was actually a thing:

@shiftkey methods running async operations should return Task and should end with Async.

— Immo Landwerth (@terrajobst) September 12, 2016

But if people are really into doing this, and are willing to put in the work to migrate things properly - rather than just break the world - then I won't stand in the way.

Feels @ryangribble @alfhenrik @dampir @haacked?

Breaks literally every piece of software written using Octokit, for literally zero tangible benefits to users of the library. Easy decision.

cc @shana @grokys @paladique for additional feels

Here is how I see this:

• When deciding on guidelines, consistency trumps virtually everything else. Developers don't just use one component of .NET, they use a ton of .NET components. So even if an entire component is exclusively async, staying consistent with other components that offer both async and sync APIs helps. It also helps in code reviews because calling a method that is suffixed with Async but is not awaited is almost always a bug.
• We do not perform breaking changes. Sometimes, guidelines come to be after a given API has shipped. That's unfortunate, which is why we have a special support group at Microsoft where we can cry, pet each other's back and drink hoppy beverages to deal with it. But that's how the cookie crumbles.

So consistency is important, but breaking people's code just because it doesn't meet certain guidelines is silly and directly interferes with the reason we have guidelines in the first place: making our customers successful. Of course, you could offer additional overloads that are properly named but then your component uses both naming conventions and code base will most likely ending up with calling a mixture. In my opinion, that's even worse because now the async suffix means nothing in Octokit.

So I'd be in favor of closing as won't fix.

/cc @stephentoub

As I recall, the reason .NET has Async suffixes all over the place is because the original API was not async, and to maintain compatibility, instead of making the API async-first (as MS wanted), they instead added the Async suffix. MS decided not to break the API and every user out there even though an async-first API was their preferred choice.

The irony of creating new APIs outside of the original framework (and octokit is not a part of the .NET class libraries, it is an independent entity that has its own identity and API design decisions) and making them sync-first with Async suffixes for the async behaviour is, well, ironic?

In my opinion, that's even worse because now the async suffix means nothing in Octokit.

@terrajobst thanks for putting it so succintly ✨

The main reason I suggested this was that when starting with the API this completely throw me off. I knew what I wanted had to be async, but I couldn't find any async methods. It's also really inconvenient not instantly knowing you need to put await in front of some code, but have to go look at the signature.
As @terrajobst said this is why coding guidelines are important and when working with multiple libraries it can really throw off a developer.

@shana I don't think the reasons you present are correct (it sounds more like rumors and doesn't really fit with other things that were in .NET before Tasks - take a look at what they did to WebClient for instance). It's just as much about readability, eases code-reviews etc. Just because we have async/await doesn't mean multithreading problems gets any easier (besides the less code needed), and it's nice when the code explicitly calls out that there's context switching, it's non-blocking, likely long-running etc.

Regarding @paulcbetts's comment:

Breaks literally every piece of software written using Octokit, for literally zero tangible benefits to users of the library

No. Consistency, predictability and readability of an API is not zero benefit. Secondly as I mentioned, when you provide the new overloads, you mark the old ones obsolete, and hide them from intellisense. That means you won't see the old overloads clouding up intellisense, they'll still work and users gets warnings telling them to move to the new ones. That also takes care of @terrajobst's worry that we'll have a mix of coding patterns throughout the code. That's quickly taken care off by cleaning up the build warnings.

@dotMorten

That also takes care of @terrajobst's worry that we'll have a mix of coding patterns throughout the code. That's quickly taken care off by cleaning up the build warnings.

I'm not an expert in Octokit, but it sounds to me as if this would mean virtually marking all methods as obsolete, which will have significant impact on the consumers. That still seems excessive to me.

That's fair. I knew it was a longshot to begin with, but my OCD just couldn't handle it 😀

I'm not an expert in Octokit, but it sounds to me as if this would mean virtually marking all methods as obsolete, which will have significant impact on the consumers.

For reference, here's how it would affect Octokit:

• 668 public methods returning Task<T>
• Of these, 96 are public async Task<T>
• 81 public methods returning Task
• Of these, 4 are public async Task

Oh, and another interesting data point - the earliest commit on this project was from late April 2012, a few months before .NET 4.5 was released. The code was extracted from GitHub for Windows, but that gives you an idea of how far back this decision probably went...

@shiftkey Task has been around since 4.0 / 2010 😜

@dotMorten true, but the new *Async APIs in the BCL weren't added until .NET 4.5

Mark every method that return Task as obsolete it means mark Octokit.NET as obsolete (Observable methods are just wrappers). And I think that this is unacceptable for now. Time for such code convention decision is gone for Octokit.

Consistency, predictability and readability of an API is not zero benefit.

I definitely agree with that, but not in case of Octokit and Async suffix. Octokit is fully asynchronous library, each method in each client is async. This .NET Framework 4.5 library and it doesn't have any legacy API that uses obsolete asynchronous programming patterns (such as APM or EAP). So, I think that in case of Octokit Async suffix not so valuable and can be omitted.

The main reason I suggested this was that when starting with the API this completely throw me off. I knew what I wanted had to be async, but I couldn't find any async methods.

Maybe, it will be nice add remark about lack of Async suffix in README.md with accent on the fact that Octokit is fully async?

This .NET Framework 4.5 library and it doesn't have any legacy API that uses obsolete asynchronous programming pattern

I don't see how that has anything to do with following the common .NET coding patterns for Task-returning methods, and it is completely ignoring the arguments for postfixing with async that has been stated above that has nothing to do with any legacy .NET.

I get the argument that at this point you don't want to change your naming mistake - the ship has sailed and it is what it is..
However I don't see how any other of the arguments presented in the entire thread against fixing the naming are valid. Those are all rooted in "I just don't like it", but that's the beauty of coding guidelines: They don't care :-)

No Async suffix for Nancy. :)

While do I see @dotMorten's point, and I'm a great believer in following style guidelines, I think @terrajobst put it best, in that not making breaking changes is more important. I certainly don't relish going through my codebases adding Async to the end of each call, and I imagine that for really heavy users of octokit this would be a real burden.

So yeah, @dotMorten has a good point, but the ship has sailed on this I think.

Not that it's really required but my 2c is definitely in agreement that the 🚢 had sailed on this

As @shiftkey said if someone honestly feels passionately enough about this to make ALL the changes in a non breaking way I'd help get the PR through but I definitely don't intend to do it 😝

No worries.
If you do change your mind, I have a code analyzer that'll auto-fix it in one big swoop. I could probably even tweak it a little to keep the old ones and mark them hidden and obsolete, so it would avoid the breaking changes.

I think @shana said it very succinctly. To prefix all methods (or even a majority) with Async is just rote, it misses the point of the convention. The point isn't to delineate asynchronous methods, it's too delineate deviations from the established api style. The established api style being either synchronous or asynchronous. Given an api with an asynchronous style, the convention should be the other way. For the deviating methods to be suffixed with Sync.

@peteraritchie The API style should follow common .net conventions. It's a huge mistake to have different APIs that you use together use different API styles. If this is the "API style" for octokit, let me be frank and say it uses the wrong style. Octokit isn't the only API one codes against in an app. But if you're fine with that, so be it.

The point isn't to delineate asynchronous methods, it's too delineate deviations from the established api style.

Sorry. But no.

Alright, that's enough.