Add new extension: DSharpPlus.Commands #1680
Conversation
|
as a first thing before i start reviewing: we should probably consider naming it something like Commands without suffix; and i believe the same pull request should also annotate |
|
Because the rest of DSharpPlus is on .NET 7, I've chosen to keep CNext and Slashies not obsolete until .NET 8 releases |
|
that'll be in a few days, and probably before merging this PR |
|
Done |
OoLunar
changed the title
|
Before any documentation work happens, i would suggest to archive old command articles, instead of removing them completely |
There was a problem hiding this comment.
overarching points:
consider renaming DSharpPlus.Commands.Commands. since it mostly deals with construction of a command tree; DSharpPlus.Commands.Trees?Done - Lunar- i really don't like multiple types in one file, it makes it harder to find these types later
- what exactly is the DX for configuring different processors? this seems a little doubtful given the implementation, but maybe you have a solution
- attributes should never contain logic, and much less throw exceptions. i believe @Instellate had a good design for checks in Introducing a new command framework #1553
- this looks like it is restricting text commands to the same or similar parameters as application commands, which i don't exactly like - application commands are relatively limited, ie in nesting depth and in not supporting overloads.
DSharpPlus.Commands/Processors/TextCommands/Parsing/DefaultTextArgumentSplicer.cs
Show resolved
Hide resolved
DSharpPlus.Commands/Processors/TextCommands/Parsing/DefaultTextArgumentSplicer.cs
Show resolved
Hide resolved
DSharpPlus.Commands/Processors/SlashCommands/SlashCommandProcessor.cs
Outdated
Show resolved
Hide resolved
DSharpPlus.Commands/Processors/BaseCommandProcessor/ICommandProcessor.cs
Outdated
Show resolved
Hide resolved
|
|
||
| public interface IArgumentConverter<TEventArgs, TOutput> : IArgumentConverter where TEventArgs : AsyncEventArgs | ||
| { | ||
| public Task<Optional<TOutput>> ConvertAsync(ConverterContext context, TEventArgs eventArgs); |
There was a problem hiding this comment.
In my experience I use argument converters for some network related calls, such as to a db or external website. Shouldn't task be used when the CPU is idling?
What do you mean by DX? |
|
developer experience, as in, how tedious is it to configure a command processor |
| } | ||
| } | ||
|
|
||
| // If we didn't find the user in the guild, try to get the user from the API. |
There was a problem hiding this comment.
I dont like making hidden calls in converters maybe we can introduce a option to configure if we should make calls here
| { | ||
| add | ||
| { | ||
| if (this.UseDefaultCommandErrorHandler) |
There was a problem hiding this comment.
I think we should toggle this.UseDefaultCommandErrorHandler when we unregister it
| this._commandErrored.Register(DefaultCommandErrorHandlerAsync); | ||
| } | ||
|
|
||
| // Attempt to get the user defined logging, otherwise setup a null logger since the D#+ Default Logger is internal. |
There was a problem hiding this comment.
I think we should open internals when merging
There was a problem hiding this comment.
we should generally revamp that logger
| } | ||
|
|
||
| [SuppressMessage("Roslyn", "CA1859", Justification = "Incorrect warning in NET 8-rc2. TODO: Open an issue in dotnet/runtime.")] | ||
| private async Task<DiscordApplicationCommandOption> ToApplicationParameterAsync(Command command, CommandParameter parameter, int? i = null) |
There was a problem hiding this comment.
Maybe a bit more describtive parameter name.
There was a problem hiding this comment.
It's... the parameter you're converting to an application parameter. How much more descriptive could it get?
There was a problem hiding this comment.
I was talking about the last parameter, its confusing when you read a long method and then theres a random i
DSharpPlus.Commands/Processors/TextCommands/Parsing/DefaultTextArgumentSplicer.cs
Show resolved
Hide resolved
it's... not exactly fast, nor is it very nice, but it works unless one actively tries to break it. this code might also break if the compiler changes the way they're emitted, in which case we're in some trouble
…ong as all extra parameters have default values
…checks for message content; warn when missing required intents
|
All changes and review comments will be moved into issues as we need to start receiving user feedback soon. Merging into nightlies. |
|
i'll take a cursory look for intolerable flaws and then sign off on this |
|
|
Summary
At the moment, both CommandsNext and SlashCommands are relatively unmaintained while containing a lot of... unoptimized code. Additionally, having these packages separate will force the consuming user to choose between text commands or interactions. This extension unifies all command frontends through command processors.
Requirements
Roadmap
Notes
Light testing has been done and core functionality is guaranteed, however more thorough testing is required.