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

v3: API design review #1340

Draft
wants to merge 10 commits into
base: blank
Choose a base branch
from
Draft

v3: API design review #1340

wants to merge 10 commits into from

Conversation

foxbot
Copy link
Member

@foxbot foxbot commented Jul 1, 2019

Hi all,

Please use this PR to leave any comments, feedback, and concerns about the new API design for v3. You are welcome to leave a comment directly on this thread, or use a GitHub suggestion on the relevant file.

Design feedback may also be left in the Discord.Net channel in Discord API.

Note that this review is focused primarily around the public API only; none of the interfaces exposed (sans the IDiscordClient) have any actual implementations yet. Implementation of the interfaces will come during a later pass.

@foxbot foxbot self-assigned this Jul 1, 2019
src/Discord.Net/Entities/Channels/IGuildChannel.cs Outdated Show resolved Hide resolved
src/Discord.Net/Entities/Channels/IGuildChannel.cs Outdated Show resolved Hide resolved
public interface IMessageChannel : IChannel
{
Task<IUserMessage> SendMessageAsync(string? text = null, bool isTTS = false, IEmbed? embed = null, RequestOptions? options = default);
Task<IUserMessage> SendFileAsync(Stream fileStream, string filename, string? text = null, bool isTTS = false, IEmbed? embed = null, RequestOptions? options = default);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could maybe considering a FileProperties class? This would mean you wouldn't need the SendFileAsync overloads/extensions.

Choose a reason for hiding this comment

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

Or as simple as SendFileOptions. Make it a struct if you're terribly worried about allocations.

Copy link

@xSke xSke Jul 20, 2019

Choose a reason for hiding this comment

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

Can we have a way to upload multiple files per message, too? I know this is supported (mobile clients do it!). Perhaps an overload taking an IEnumerable<SendFileOptions>.

Task SyncPermissionsAsync(RequestOptions? options = default);

// review: do we want long, paramaterized methods like this, or would we rather a CreateInviteProperties
Task CreateInviteAsync(int? maxAge = 86400,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a properties class is much cleaner.

Choose a reason for hiding this comment

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

I also lean towards CreateInviteProperties or CreateInviteOptions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

// todo: docs
namespace Discord
{
// review: marker interfaces suck! should we just have an "IsNews" property on ITextChannel?
Copy link
Contributor

Choose a reason for hiding this comment

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

IsNews property, yes.

Choose a reason for hiding this comment

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

Same comment as above. If there is no functional/operational difference in the API, a property is simpler.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So what happens when discord decides to add (more) special functionality to news channels? (they can switch between news and text types) Or, are we still sticking to treating them just like text channels and not supporting them (like store channels)?


Task ModifyAsync(MemberProperties properties, RequestOptions? options = default);

// review: should these two be extensions on IGuild?
Copy link
Contributor

Choose a reason for hiding this comment

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

There should at least be an id version of it on IGuild.

Choose a reason for hiding this comment

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

That thought should apply to almost all API functionality. Except, perhaps, when there should be a "does this ID exist" check, in which case performing the Get() call serves that purpose.

{
string Username { get; }
string Discriminator { get; }
// review: does anyone use DiscriminatorValue? seems like a waste of memory
Copy link
Contributor

Choose a reason for hiding this comment

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

It is a waste, yes.

Choose a reason for hiding this comment

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

I can find no usage of it that I wouldn't be happy to replace.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to make this a ushort and people can just ToString() it if needed? I feel like it could stay consistent since discriminators are numbers.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that it would be most appropriate to keep Discriminators strings to match the API: https://discordapp.com/developers/docs/resources/user#user-object
There's less risk of things breaking should they change what discriminators can be.

public Optional<IEnumerable<SnowflakeOrEntity<IRole>>> Roles { get; set; }
public Optional<SnowflakeOrEntity<IVoiceChannel>?> VoiceChannel { get; set; }

public Model ToWumpus()
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this meant to be internal?


event Action<IMessage> MessageCreated;
// todo: implement the rest of the events
// review: event classes or just Action<A,B,C> types
Copy link
Contributor

Choose a reason for hiding this comment

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

the EventArgs pattern is probably better.

Choose a reason for hiding this comment

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

EventArgs or similar please. For one, we do basically that anyway, in a wrapper layer around the client. For two, it's WAY better from a documentation standpoint. For example, GuildMemberUpdated today has a signature of

public event Func<SocketGuildUser, SocketGuildUser, Task> GuildMemberUpdated;

If I don't already know how this event works, and want to know, which user is the "old" version and which is the "new" one, the only way to really know for sure is to go dig through the source on github and see that when it's actually invoked, it's...

await TimedInvokeAsync(_guildMemberUpdatedEvent, nameof(GuildMemberUpdated), before, user).ConfigureAwait(false);


ValueTask<IUser> GetUserAsync(ulong id, StateBehavior stateBehavior, RequestOptions? options);
// review: IReadOnlyCollection is proper collection type here?
ValueTask<IReadOnlyCollection<IUser>> GetUsersAsync(StateBehavior stateBehavior, RequestOptions? options);
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. As long as the immutable collections aren't used (they're very slow and memory heavy).

Choose a reason for hiding this comment

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

Not sure I agree with that assessment of immutables, in general, but I definitely agree that IReadOnlyCollection is the best bet here. Immutables would be appropriate if you KNOW your consumer is going to be doing a lot of collection manipulation, and also wants immutability.

The other option would be IReadOnlyList but do we really expect consumers to want indexing by position?

src/Discord.Net/DiscordClientBuilder.cs Outdated Show resolved Hide resolved
src/Discord.Net/Entities/Activities/IActivity.cs Outdated Show resolved Hide resolved
{
public class DiscordClientConfig
{
public string? Token { get; set; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this mean that TokenType is dead, and will be assumed based on the type of client that is used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not specifically, I just hadn't implemented it yet. Though, this client will only support bot tokens, as it makes more sense for OAuth users to just use Wumpus directly.

{
public string? Token { get; set; }

public int Shard { get; set; } = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are the sharded client and websocket client going to be merged?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we're just wrapping over a WumpusGatewayClient and not actually implementing any client logic ourselves here, I think it seems reasonable to allow an IDiscordClient to support multiple shards.

Task SyncPermissionsAsync(RequestOptions? options = default);

// review: do we want long, paramaterized methods like this, or would we rather a CreateInviteProperties
Task CreateInviteAsync(int? maxAge = 86400,
Copy link
Collaborator

Choose a reason for hiding this comment

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

ulong EveryoneRoleId { get; }
ulong OwnerId { get; }

int AFKTimeout { get; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

timespan?


ValueTask<IReadOnlyCollection<IAttachedEmote>> GetEmotesAsync(StateBehavior stateBehavior = default, RequestOptions? options = default);
ValueTask<IAttachedEmote> GetEmoteAsync(StateBehavior stateBehavior = default, RequestOptions? options = default);
Task CreateEmoteAsync(string name, Image image, RequestOptions? options = default);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this consume an EmoteBuilder instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

EmoteBuilder is a static class for constructing IEmote/IGuildEmote from strings; see above comment regarding DiscordClientBuilder. I'll probably just replace this with IEmote.Parse/IGuildEmote.Parse


bool IsRevoked { get; }
bool IsTemporary { get; }
int? MaxAge { get; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Timespan?

bool IsManaged { get; }
bool IsMentionable { get; }

GuildPermissions Permissions { get; } // review: use wump permissions directly or wrap?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see any point in wrapping imo

IMemberPresence Presence { get; }

DateTimeOffset? JoinedAt { get; }
string Nickname { get; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: thoughts on a DisplayName extension that does Nickname ?? Username, since it's asked for so often

Choose a reason for hiding this comment

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

Seconded.

Copy link
Member Author

Choose a reason for hiding this comment

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

sounds good to me, I guess we can just make this a default interface implementation, since it doesn't look like extension everything is going to make the cut for c#8

Copy link

@JakenVeina JakenVeina left a comment

Choose a reason for hiding this comment

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

Thanks a ton for the opportunity to comment.

// todo: docs
namespace Discord
{
// review: another marker interface 😕

Choose a reason for hiding this comment

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

I think it's appropriate to split the different types of entities via the typing system, rather than simple properties, when the set of functionality for that entity diverges. I.E. when there are operations that can be performed only upon a category channel, and operations that cannot be performed upon a category channel, even if they don't appear directly upon the interface. Such operations can then be defined to operate specifically upon ICategoryChannel, which provides a more self-documented, foolproof API.

If no such operations exist for ICategoryChannel then it probably makes more sense as just a property.

public interface IMessageChannel : IChannel
{
Task<IUserMessage> SendMessageAsync(string? text = null, bool isTTS = false, IEmbed? embed = null, RequestOptions? options = default);
Task<IUserMessage> SendFileAsync(Stream fileStream, string filename, string? text = null, bool isTTS = false, IEmbed? embed = null, RequestOptions? options = default);

Choose a reason for hiding this comment

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

Or as simple as SendFileOptions. Make it a struct if you're terribly worried about allocations.

Task SyncPermissionsAsync(RequestOptions? options = default);

// review: do we want long, paramaterized methods like this, or would we rather a CreateInviteProperties
Task CreateInviteAsync(int? maxAge = 86400,

Choose a reason for hiding this comment

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

I also lean towards CreateInviteProperties or CreateInviteOptions.

// todo: docs
namespace Discord
{
// review: marker interfaces suck! should we just have an "IsNews" property on ITextChannel?

Choose a reason for hiding this comment

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

Same comment as above. If there is no functional/operational difference in the API, a property is simpler.

src/Discord.Net/Entities/Channels/ITextChannel.cs Outdated Show resolved Hide resolved

event Action<IMessage> MessageCreated;
// todo: implement the rest of the events
// review: event classes or just Action<A,B,C> types

Choose a reason for hiding this comment

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

EventArgs or similar please. For one, we do basically that anyway, in a wrapper layer around the client. For two, it's WAY better from a documentation standpoint. For example, GuildMemberUpdated today has a signature of

public event Func<SocketGuildUser, SocketGuildUser, Task> GuildMemberUpdated;

If I don't already know how this event works, and want to know, which user is the "old" version and which is the "new" one, the only way to really know for sure is to go dig through the source on github and see that when it's actually invoked, it's...

await TimedInvokeAsync(_guildMemberUpdatedEvent, nameof(GuildMemberUpdated), before, user).ConfigureAwait(false);

event Action Disconnected;
event Action Ready;

event Action<IChannel> ChannelCreated;

Choose a reason for hiding this comment

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

Are events no longer asynchronous? If not, presumably we become responsible for dispatching the event to a new async context, if desired?

I'm totally cool with that, since that's what we do currently anyway, but it might be a good idea to have a built-in mechanism to do said dispatching, for beginner consumers. Something like an injectable IDiscordEventDispatcher, with a couple built-in implementations?

Copy link
Member Author

@foxbot foxbot Jul 2, 2019

Choose a reason for hiding this comment

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

Yes, users are now responsible for dispatching to an async context. (see the included example)

I'm not familiar with what you mean by providing something like an IDiscordEventDispatcher, if you could link me to an example of how that might look I would be happy to include it.

Choose a reason for hiding this comment

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

In my mind, the ideal way to handle discord events is the same way ASP.NET handles HTTP requests: each incoming message spawns a new "logical" thread. I.E. a new IServiceScope in the DI system, and a spawned Task that is simply not awaited, so that it runs in parallel to other "logical" threads.

For the current Discord.NET version, we do this with several pieces fitting together:
https://github.com/discord-csharp/MODiX/blob/master/Modix.Common/Messaging/MessageDispatcher.cs
https://github.com/discord-csharp/MODiX/blob/master/Modix.Common/Messaging/INotificationHandler.cs
https://github.com/discord-csharp/MODiX/blob/master/Modix.Services/Core/DiscordSocketListeningBehavior.cs

You can see how subscribing to each event we need, wrapping up the args into an object, and then handing off the work to the dispatcher is a tad tedious.

An IDiscordEventDispatcher could look like the following...

public interface IDiscordEventDispatcher
{
    Task DispatchEvent(IEnumerable<Func<Task>> eventHandlers);
}

Where you could have the following built-in implementations that provide behavior similar to Discord.Commands.RunMode.

public class SyncDiscordEventDispatcher
    : IDiscordEventDispatcher
{
    public Task DispatchEvent(IEnumerable<Func<Action>> eventHandlers)
    {
        foreach(var eventHandler in eventHandlers)
            await eventHandler.Invoke();
    }
}
public class ParallelDiscordEventDispatcher
    : IDiscordEventDispatcher
{
    public Task DispatchEvent(IEnumerable<Func<Task>> eventHandlers)
    {
        foreach(var eventHandler in eventHandlers)
            _ = eventAction.Invoke();
        return Task.CompletedTask;
    }
}

You can throw in some basic error handling around the Tasks that aren't awaited, or leave that to the consumer.

In our case, we'd implement a custom dispatcher that also handles DI scoping, since Discord.NET doesn't have any built-in DI. Which, actually, now that I think about it, wouldn't fit with the above interface definition.

So, if you're interested at all, it definitely needs some brainstorming. But, I think the concept of handling messages the way ASP.NET handles HTTP requests would be appealing to a lot of consumers.


namespace Discord
{
internal class ConcurrentMemoryStateProvider : IStateProvider

Choose a reason for hiding this comment

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

This makes me smile.


ValueTask<IUser> GetUserAsync(ulong id, StateBehavior stateBehavior, RequestOptions? options);
// review: IReadOnlyCollection is proper collection type here?
ValueTask<IReadOnlyCollection<IUser>> GetUsersAsync(StateBehavior stateBehavior, RequestOptions? options);

Choose a reason for hiding this comment

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

Not sure I agree with that assessment of immutables, in general, but I definitely agree that IReadOnlyCollection is the best bet here. Immutables would be appropriate if you KNOW your consumer is going to be doing a lot of collection manipulation, and also wants immutability.

The other option would be IReadOnlyList but do we really expect consumers to want indexing by position?

/// abide by design implications of this flag. Once Discord.Net has called out to the state provider with this
/// flag, it is out of our control whether or not an async method is evaluated.
/// </remarks>
SyncOnly = 1,

Choose a reason for hiding this comment

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

Possible need for clarification: What's the difference between SyncOnly and CacheOnly?

Copy link
Member Author

Choose a reason for hiding this comment

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

SyncOnly requests (can't enforce) that a state provider never generates an async-state machine, since all state lookups are ValueTasks.

CacheOnly requests that a state provider doesn't contact Discord when retrieving an entity.

The motivation for implementing this might be if you're using an additional caching layer for long-term storage, such as redis, where a state provider might need to make an async call to retrieve information. If you're in a context where the ValueTask must remain synchronous (maybe in a TypeReader, or if you're .Result'ing), you could use SyncOnly.

Choose a reason for hiding this comment

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

That's an excellent thought. You should probably just take that last paragraph and inject it into the XML.

{
public interface IDiscordClient
{
event Action Connected;
Copy link

Choose a reason for hiding this comment

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

What's the logic behind having these defined as events? Personally, I'd much prefer something like exposing the AsyncEvent<T> fields from v2 (which are private, and used internally anyway). This way, you can work with events as first-class objects, and e.g. do something like public async Task<T> WaitFor(this AsyncEvent<T>) like in discord.py - super useful for making interactive stuff.

foxbot and others added 9 commits January 5, 2020 18:11
incomplete, still needs receive handling
gateway is yet to be designed
basic enough to test some stuff with

fixed some bugs with disposables in the process
Optional struct needs work still, + writing the converter for it is
going to be a headache
does not support property omission at this time, will need to be added
later using a separate converter and base marker class.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

"git failing to recognize gpg key, this identity is still valid"
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEErbDRxgZ77MPT+ajAOrLKmA3cYakFAl4bthEACgkQOrLKmA3c
YamuWw/7Bn/Ks0mTRN3tg3Z/voETJ/8JQZXJEiW7wwv8c7nSOemxRNB/Tmzo3kzC
N6T5fH7Gep4o4iA7CfJ5CZtx+OY92OpyBwsJgkNvANVpjXWCeDaww0Ci5dyVwFUk
fFq21l6p2sbM6PB9sEOCvryeIOrgkqBl915MkAlj+/UtnAQ9qFhomIGNLPPFeYOS
eaHWjZF6ArbF5NMaOhboDDCIl2nCf+RGEetDoBP2BRaIf+eOyl0lGyQqiY1mNqkD
DX8nmcaY5/Lnxhf3pwmYZbqKBPQt5R2FxmqWTg5ey0R4//izE4TJ54nlhdSnTZpH
7ZligmR9rQFdQ5jbSq6cIclo9i988ELHKBgt8mG3SiC4AT0+SBXRpPRBitkA0CPb
O4W8J0HrbSFmILx9Zvuy72KC/Zzo+SOS8257S35ihosrlyupcR4zladVcIviAPWk
Ovpy85W4uxPdWc6zkMOZSx9OiYFYkNlK/QdNJBXGg7LLcaLf8p33lj+T8UXa7dyC
Sw/pW5RL1FYalh7iXF55ylJrKo+oySBejods+ATnmYG4JMywO+GNCE+XLCcDpoBx
9H2z0qJNb5Dgkc4cRulKwYEoT+LQKUhLFdj4wNEqE8mBw0ZoxUiBBqOD1TiZr2mf
1AFQVS/AeOc03t25OfmhNz026OAGy01bjeHr09deT20dsssEpQY=
=n76m
-----END PGP SIGNATURE-----
@csmir
Copy link
Collaborator

csmir commented Aug 2, 2022

@quinchs are there any plans to act on this PR in the future?

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.