v3: API design review #1340

foxbot · 2019-07-01T22:42:15Z

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.

src/Discord.Net/Entities/Channels/IGuildChannel.cs

k-boyle · 2019-07-01T22:53:19Z

src/Discord.Net/Entities/Channels/IMessageChannel.cs

+    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);


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

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

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>.

k-boyle · 2019-07-01T22:54:12Z

src/Discord.Net/Entities/Channels/INestedChannel.cs

+        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,


I think a properties class is much cleaner.

I also lean towards CreateInviteProperties or CreateInviteOptions.

So, should we do the same for stuff like ChannelPermissions, lest we forget the best url: https://docs.stillu.cc/api/Discord.ChannelPermissions.html#Discord_ChannelPermissions__ctor_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_

k-boyle · 2019-07-01T22:54:30Z

src/Discord.Net/Entities/Channels/INewsChannel.cs

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


IsNews property, yes.

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

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)?

k-boyle · 2019-07-01T23:05:44Z

src/Discord.Net/Entities/Users/IMember.cs

+
+        Task ModifyAsync(MemberProperties properties, RequestOptions? options = default);
+
+        // review: should these two be extensions on IGuild?


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

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.

k-boyle · 2019-07-01T23:06:13Z

src/Discord.Net/Entities/Users/IUser.cs

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


It is a waste, yes.

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

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.

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.

k-boyle · 2019-07-01T23:07:06Z

src/Discord.Net/Entities/Users/MemberProperties.cs

+        public Optional<IEnumerable<SnowflakeOrEntity<IRole>>> Roles { get; set; }
+        public Optional<SnowflakeOrEntity<IVoiceChannel>?> VoiceChannel { get; set; }
+
+        public Model ToWumpus()


Isn't this meant to be internal?

k-boyle · 2019-07-01T23:07:49Z

src/Discord.Net/IDiscordClient.cs

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


the EventArgs pattern is probably better.

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);

k-boyle · 2019-07-01T23:08:53Z

src/Discord.Net/State/IStateProvider.cs

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


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

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?

examples/0_Using_the_Client/Program.cs

src/Discord.Net/DiscordClientBuilder.cs

src/Discord.Net/Entities/Activities/IActivity.cs

Chris-Johnston · 2019-07-02T02:07:29Z

src/Discord.Net/DiscordClientConfig.cs

+{
+    public class DiscordClientConfig
+    {
+        public string? Token { get; set; }


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

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.

Chris-Johnston · 2019-07-02T02:08:22Z

src/Discord.Net/DiscordClientConfig.cs

+    {
+        public string? Token { get; set; }
+
+        public int Shard { get; set; } = 0;


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

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.

Chris-Johnston · 2019-07-02T02:15:14Z

src/Discord.Net/Entities/Channels/INestedChannel.cs

+        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,


So, should we do the same for stuff like ChannelPermissions, lest we forget the best url: https://docs.stillu.cc/api/Discord.ChannelPermissions.html#Discord_ChannelPermissions__ctor_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_

Chris-Johnston · 2019-07-02T02:24:38Z

src/Discord.Net/Entities/Guilds/IGuild.cs

+        ulong EveryoneRoleId { get; }
+        ulong OwnerId { get; }
+
+        int AFKTimeout { get; }


Chris-Johnston · 2019-07-02T02:30:15Z

src/Discord.Net/Entities/Guilds/IGuild.cs

+
+        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);


Should this consume an EmoteBuilder instead?

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

Chris-Johnston · 2019-07-02T02:32:02Z

src/Discord.Net/Entities/Invites/IInviteMetadata.cs

+
+        bool IsRevoked { get; }
+        bool IsTemporary { get; }
+        int? MaxAge { get; }


Chris-Johnston · 2019-07-02T02:37:07Z

src/Discord.Net/Entities/Roles/IRole.cs

+        bool IsManaged { get; }
+        bool IsMentionable { get; }
+
+        GuildPermissions Permissions { get; } // review: use wump permissions directly or wrap?


I don't see any point in wrapping imo

Chris-Johnston · 2019-07-02T02:41:00Z

src/Discord.Net/Entities/Users/IMember.cs

+        IMemberPresence Presence { get; }
+
+        DateTimeOffset? JoinedAt { get; }
+        string Nickname { get; }


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

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

JakenVeina

Thanks a ton for the opportunity to comment.

JakenVeina · 2019-07-02T01:17:44Z

src/Discord.Net/Entities/Channels/ICategoryChannel.cs

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


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.

JakenVeina · 2019-07-02T01:28:06Z

src/Discord.Net/Entities/Channels/IMessageChannel.cs

+    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);


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

JakenVeina · 2019-07-02T01:29:14Z

src/Discord.Net/Entities/Channels/INestedChannel.cs

+        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,


I also lean towards CreateInviteProperties or CreateInviteOptions.

JakenVeina · 2019-07-02T01:30:19Z

src/Discord.Net/Entities/Channels/INewsChannel.cs

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


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

JakenVeina · 2019-07-02T02:39:50Z

src/Discord.Net/IDiscordClient.cs

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


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);

JakenVeina · 2019-07-02T02:42:47Z

src/Discord.Net/IDiscordClient.cs

+        event Action Disconnected;
+        event Action Ready;
+
+        event Action<IChannel> ChannelCreated;


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?

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.

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.

JakenVeina · 2019-07-02T02:44:07Z

src/Discord.Net/State/ConcurrentMemoryStateProvider.cs

+
+namespace Discord
+{
+    internal class ConcurrentMemoryStateProvider : IStateProvider


This makes me smile.

JakenVeina · 2019-07-02T02:46:49Z

src/Discord.Net/State/IStateProvider.cs

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


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?

JakenVeina · 2019-07-02T02:49:41Z

src/Discord.Net/State/StateBehavior.cs

+        ///     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,


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

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.

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

xSke · 2019-07-20T21:25:30Z

src/Discord.Net/IDiscordClient.cs

+{
+    public interface IDiscordClient
+    {
+        event Action Connected;


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.

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 · 2022-08-02T09:28:24Z

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

foxbot added the discussion label Jul 1, 2019

foxbot self-assigned this Jul 1, 2019

k-boyle reviewed Jul 1, 2019

View reviewed changes

monoclex reviewed Jul 2, 2019

View reviewed changes

examples/0_Using_the_Client/Program.cs Outdated Show resolved Hide resolved

Chris-Johnston reviewed Jul 2, 2019

View reviewed changes

JakenVeina reviewed Jul 2, 2019

View reviewed changes

xSke reviewed Sep 21, 2019

View reviewed changes

meta: destroy repo

24bcf3b

foxbot force-pushed the next branch from 0cf14da to 24bcf3b Compare January 5, 2020 22:46

foxbot and others added 9 commits January 5, 2020 18:11

meta: add project structure

523da0b

net: design socket

f7c212d

incomplete, still needs receive handling gateway is yet to be designed

api: design rest and core

97ba033

net: add token support to rest client

c51bb4c

sample: add idn repl (Interactive Discord.Net)

0df54c6

basic enough to test some stuff with fixed some bugs with disposables in the process

util: add logging

18438e2

rest: write out channel model + its dependencies

5e6f977

Optional struct needs work still, + writing the converter for it is going to be a headache

rest: add webhook model (#1435)

68f89bf

csmir mentioned this pull request Feb 14, 2022

Add DisplayName property to IGuildUser. #2107

Merged


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

		// review: should these two be extensions on IGuild?

v3: API design review #1340

Are you sure you want to change the base?

v3: API design review #1340

Conversation

foxbot commented Jul 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xSke Jul 20, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakenVeina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foxbot Jul 2, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csmir commented Aug 2, 2022

xSke Jul 20, 2019 •

edited

foxbot Jul 2, 2019 •

edited