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.

Sign up for GitHub

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

Jump to bottom

Paginate reactions - solved #1007 #1022

Merged
merged 14 commits into from
May 24, 2018
Merged

Paginate reactions - solved #1007 #1022

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
c5676a1
Cleaned up and refactored slightly
HelpfulStranger999 Mar 25, 2018
4c36ad5
Resolves #971
HelpfulStranger999 Mar 26, 2018
17c332f
Adds support for default avatars and resolves #971
HelpfulStranger999 Mar 26, 2018
0862674
Amendment
HelpfulStranger999 Mar 26, 2018
a394224
Final amendment
HelpfulStranger999 Mar 26, 2018
f3b8419
Merge remote-tracking branch 'upstream/dev' into dev
HelpfulStranger999 Apr 6, 2018
18b7c21
Merge remote-tracking branch 'upstream/dev' into dev
HelpfulStranger999 Apr 7, 2018
57c55a7
Paginating reactions
HelpfulStranger999 Apr 7, 2018
87449cd
Amendments based on feedback
HelpfulStranger999 Apr 8, 2018
e819eac
Further amendment based on review
HelpfulStranger999 Apr 8, 2018
64c8eb4
Final(?) amendment
HelpfulStranger999 Apr 8, 2018
63da572
Removes default limit and after user id
HelpfulStranger999 May 13, 2018
424259f
Removes fromUserId; cleans up model creation; replaces function with …
HelpfulStranger999 May 14, 2018
dbba84d
Merge branch 'dev' into paginate-reactions
HelpfulStranger999 May 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/Discord.Net.Core/DiscordConfig.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System.Reflection;
using System.Reflection;

namespace Discord
{
Expand All @@ -20,6 +20,7 @@ public class DiscordConfig
public const int MaxMessagesPerBatch = 100;
public const int MaxUsersPerBatch = 1000;
public const int MaxGuildsPerBatch = 100;
public const int MaxUserReactionsPerBatch = 100;

/// <summary> Gets or sets how a request should act in the case of an error, by default. </summary>
public RetryMode DefaultRetryMode { get; set; } = RetryMode.AlwaysRetry;
Expand Down
4 changes: 2 additions & 2 deletions src/Discord.Net.Core/Entities/Messages/IUserMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using System.Collections.Generic;
using System.Threading.Tasks;

Expand All @@ -23,7 +23,7 @@ public interface IUserMessage : IMessage
/// <summary> Removes all reactions from this message. </summary>
Task RemoveAllReactionsAsync(RequestOptions options = null);
/// <summary> Gets all users that reacted to a message with a given emote </summary>
Task<IReadOnlyCollection<IUser>> GetReactionUsersAsync(IEmote emoji, int limit = 100, ulong? afterUserId = null, RequestOptions options = null);
IAsyncEnumerable<IReadOnlyCollection<IUser>> GetReactionUsersAsync(IEmote emoji, int limit = 100, ulong? afterUserId = null, RequestOptions options = null);
Copy link
Member

Choose a reason for hiding this comment

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

Make limit required and remove afterUserId

Copy link
Member

Choose a reason for hiding this comment

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

Why make it required when we dont do that for GetMessagesAsync that follow a similar pattern? (Could change the = 100 to = DiscordConfig.MaxUserReactionsPerBatch tho)
And why remove an option that the discord api gives to us?

Copy link
Member

Choose a reason for hiding this comment

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

It looks like GetMessagesAsync is the only one where we have a default: everywhere else limit is required (and even in IMessageChannel, limit is required!) or is not present at all.


/// <summary> Transforms this message's text into a human readable form by resolving its tags. </summary>
string Resolve(
Expand Down
2 changes: 1 addition & 1 deletion src/Discord.Net.Rest/API/Rest/GetReactionUsersParams.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
namespace Discord.API.Rest
namespace Discord.API.Rest
{
internal class GetReactionUsersParams
{
Expand Down
6 changes: 3 additions & 3 deletions src/Discord.Net.Rest/DiscordRestApiClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -618,15 +618,15 @@ public async Task<IReadOnlyCollection<User>> GetReactionUsersAsync(ulong channel
Preconditions.NotNullOrWhitespace(emoji, nameof(emoji));
Preconditions.NotNull(args, nameof(args));
Preconditions.GreaterThan(args.Limit, 0, nameof(args.Limit));
Preconditions.AtMost(args.Limit, DiscordConfig.MaxUsersPerBatch, nameof(args.Limit));
Preconditions.AtMost(args.Limit, DiscordConfig.MaxUserReactionsPerBatch, nameof(args.Limit));
Preconditions.GreaterThan(args.AfterUserId, 0, nameof(args.AfterUserId));
options = RequestOptions.CreateOrClone(options);

int limit = args.Limit.GetValueOrDefault(int.MaxValue);
int limit = args.Limit.GetValueOrDefault(DiscordConfig.MaxUserReactionsPerBatch);
ulong afterUserId = args.AfterUserId.GetValueOrDefault(0);

var ids = new BucketIds(channelId: channelId);
Expression<Func<string>> endpoint = () => $"channels/{channelId}/messages/{messageId}/reactions/{emoji}";
Expression<Func<string>> endpoint = () => $"channels/{channelId}/messages/{messageId}/reactions/{emoji}?limit={limit}&after={afterUserId}";
return await SendAsync<IReadOnlyCollection<User>>("GET", endpoint, ids, options: options).ConfigureAwait(false);
}
public async Task AckMessageAsync(ulong channelId, ulong messageId, RequestOptions options = null)
Expand Down
43 changes: 38 additions & 5 deletions src/Discord.Net.Rest/Entities/Messages/MessageHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,46 @@ public static async Task RemoveAllReactionsAsync(IMessage msg, BaseDiscordClient
await client.ApiClient.RemoveAllReactionsAsync(msg.Channel.Id, msg.Id, options);
}

public static async Task<IReadOnlyCollection<IUser>> GetReactionUsersAsync(IMessage msg, IEmote emote,
public static IAsyncEnumerable<IReadOnlyCollection<IUser>> GetReactionUsersAsync(IMessage msg, IEmote emote,
Copy link
Member

Choose a reason for hiding this comment

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

Remove GetReactionUserParams func here and add ulong? from, int? limit params instead

Action<GetReactionUsersParams> func, BaseDiscordClient client, RequestOptions options)
{
var args = new GetReactionUsersParams();
func(args);
string emoji = (emote is Emote e ? $"{e.Name}:{e.Id}" : emote.Name);
return (await client.ApiClient.GetReactionUsersAsync(msg.Channel.Id, msg.Id, emoji, args, options).ConfigureAwait(false)).Select(u => RestUser.Create(client, u)).ToImmutableArray();
Preconditions.NotNull(emote, nameof(emote));
var emoji = (emote is Emote e ? $"{e.Name}:{e.Id}" : emote.Name);
Copy link
Member

Choose a reason for hiding this comment

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

Better check for nulls before (msg and emote).
This will throw if emote is null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The message shouldn't be null, since the whole class is internal, and the places where it is called (Message objects) can't be null else Nullrefs will be thrown before it enters that method. So is it strictly necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or is it standard throughout the rest of the library?

Copy link
Member

Choose a reason for hiding this comment

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

You do have a point about the message, but emote could still receive null and throw.


var arguments = new GetReactionUsersParams();
Copy link
Member

Choose a reason for hiding this comment

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

Remove this and the func invocation

func(arguments);

return new PagedAsyncEnumerable<IUser>(
DiscordConfig.MaxUserReactionsPerBatch,
async (info, ct) =>
{
var args = new GetReactionUsersParams();
Copy link
Member

Choose a reason for hiding this comment

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

Replace with ctor which sets Limits here, and remove func invocation

func(args);
args.Limit = info.PageSize;

Copy link
Member

@SubZero0 SubZero0 Apr 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

args.Limit = info.PageSize;
To request the correct amount.

if (info.Position != null)
args.AfterUserId = info.Position.Value;
Copy link
Member

Choose a reason for hiding this comment

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

The correct way imo would be creating a local "args" with the page size and the position.


var models = await client.ApiClient.GetReactionUsersAsync(msg.Channel.Id, msg.Id, emoji, args, options).ConfigureAwait(false);
var builder = ImmutableArray.CreateBuilder<IUser>();
Copy link
Member

Choose a reason for hiding this comment

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

IMO this should be replaced with models.Select and you should call ToImmutableArray on it


foreach (var model in models)
builder.Add(RestUser.Create(client, model));

return builder.ToImmutable();
},
nextPage: (info, lastPage) =>
{
if (lastPage.Count != DiscordConfig.MaxUsersPerBatch)
return false;

info.Position = lastPage.Max(x => x.Id);
return true;
},
start: arguments.AfterUserId.IsSpecified ? arguments.AfterUserId.Value : (ulong?)null,
count: arguments.Limit.GetValueOrDefault(DiscordConfig.MaxUserReactionsPerBatch)
Copy link
Member

Choose a reason for hiding this comment

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

Replace these with from and limit suggested earlier

);

}

public static async Task PinAsync(IMessage msg, BaseDiscordClient client,
Expand Down
2 changes: 1 addition & 1 deletion src/Discord.Net.Rest/Entities/Messages/RestUserMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ public Task RemoveReactionAsync(IEmote emote, IUser user, RequestOptions options
=> MessageHelper.RemoveReactionAsync(this, user, emote, Discord, options);
public Task RemoveAllReactionsAsync(RequestOptions options = null)
=> MessageHelper.RemoveAllReactionsAsync(this, Discord, options);
public Task<IReadOnlyCollection<IUser>> GetReactionUsersAsync(IEmote emote, int limit = 100, ulong? afterUserId = null, RequestOptions options = null)
public IAsyncEnumerable<IReadOnlyCollection<IUser>> GetReactionUsersAsync(IEmote emote, int limit = 100, ulong? afterUserId = null, RequestOptions options = null)
Copy link
Member

Choose a reason for hiding this comment

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

Make limit required and remove afterUserId

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which should afterUserId go then?

Copy link
Member

Choose a reason for hiding this comment

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

It's handled as part of the async enumerable returned, so it's unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does the parameter get passed to MessageHelper.GetReactionUsersAsync then?

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't need to be; our async enumerable code handles it automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohhhh, I see what you mean, where do I get the starting point for the Paged Enumerable then or should I leave that out?

Copy link
Member

Choose a reason for hiding this comment

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

The paged enumerable defaults to 0, which would retrieve all of the results anyway.

=> MessageHelper.GetReactionUsersAsync(this, emote, x => { x.Limit = limit; x.AfterUserId = afterUserId ?? Optional.Create<ulong>(); }, Discord, options);


Expand Down
2 changes: 1 addition & 1 deletion src/Discord.Net.WebSocket/Entities/Messages/SocketUserMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ public Task RemoveReactionAsync(IEmote emote, IUser user, RequestOptions options
=> MessageHelper.RemoveReactionAsync(this, user, emote, Discord, options);
public Task RemoveAllReactionsAsync(RequestOptions options = null)
=> MessageHelper.RemoveAllReactionsAsync(this, Discord, options);
public Task<IReadOnlyCollection<IUser>> GetReactionUsersAsync(IEmote emote, int limit = 100, ulong? afterUserId = null, RequestOptions options = null)
public IAsyncEnumerable<IReadOnlyCollection<IUser>> GetReactionUsersAsync(IEmote emote, int limit = 100, ulong? afterUserId = null, RequestOptions options = null)
Copy link
Member

Choose a reason for hiding this comment

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

Make limit required and remove afterUserId

=> MessageHelper.GetReactionUsersAsync(this, emote, x => { x.Limit = limit; x.AfterUserId = afterUserId ?? Optional.Create<ulong>(); }, Discord, options);

public Task PinAsync(RequestOptions options = null)
Expand Down