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

Attempts to resolve #961 #962

Merged
merged 10 commits into from
Mar 15, 2018
Merged

Attempts to resolve #961 #962

merged 10 commits into from
Mar 15, 2018

Conversation

Seeker1437
Copy link
Contributor

@Seeker1437 Seeker1437 commented Feb 23, 2018

fixes #961

@foxbot
Copy link
Member

foxbot commented Feb 24, 2018

All async methods in the library should be suffixed with Async.

@Seeker1437
Copy link
Contributor Author

Seeker1437 commented Feb 26, 2018

I think everything is ready to be reviewed now, I double checked after work to make sure.


public static async Task<int> GetRecommendShardCountAsync(BaseDiscordClient client, RequestOptions options)
{
var response = await client.ApiClient.GetBotGatewayAsync(options);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs ConfigureAwait

@@ -459,6 +471,8 @@ internal virtual void Dispose(bool disposing)
endpoint = () => $"channels/{channelId}/messages?limit={limit}";
return await SendAsync<IReadOnlyCollection<Message>>("GET", endpoint, ids, options: options).ConfigureAwait(false);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nitpick but this whitespace seems unnecessary

@@ -277,6 +277,18 @@ internal virtual void Dispose(bool disposing)
await SendAsync("GET", () => "auth/login", new BucketIds(), options: options).ConfigureAwait(false);
}

//Gateway
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why were these moved to REST instead of being left with gateway?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was told it was a better place for them because they themselves are REST requests and should be accessible from that client as well.

I this it should be this way too. As before mentioned, since I have my bot set up in a more advanced configuration, it would be nice to be able to just access the data from the rest client so I can pass it to the individual clients who will then be establishing docket client connections from there.

I with it being there it is more open and therefor more flexible, even though this appears to be an edge case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fair to me

@foxbot foxbot merged commit fc5e70c into discord-net:dev Mar 15, 2018
FiniteReality pushed a commit to FiniteReality/Discord.Net that referenced this pull request May 5, 2018
* Move REST requests to appropiate class

* Add call to ClientHelper and expose to public API

* Expose shard count request in public api

* Expose method from interface

* Update sharded client to utilize the new method

* Method is already implemented in a base class

* Refactor name to fit pattern for methods returning a `Task`

* Adds missing ConfigureAwait

* Corrects unnecessary whitespace

* Removes unneeded whitespace
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Expose Shards property from GetBotGatewayResponse object in some way.
2 participants