Add new extension: DSharpPlus.Commands #1680

OoLunar · 2023-11-12T19:57:05Z

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

Documentation
Argument converter invocation improvements.
Bug testing
Some form of automated testing for parsing commands or even executing them without a connection to Discord. This has partially been done in the original repository: OoLunar/DSharpPlus.CommandAll:/tests/Commands/CommandBuilderTests.cs
Http Interactions, which was previously difficult due to the restrictions of internal entities.

Roadmap

Feature	Currently Supported
Text Commands	Yes
Slash Commands	Yes
Command Aliases	Text Commands
Generated Aliases	No
Command Groups	Yes
Custom Argument Type Converters	Yes
Per Argument Type Converters	Yes*
Built In Help Command	No
Params (Auto Generated Arguments)	Yes
XML Documentation	No
Translation Classes	Yes
Pre-Execution Checks	Yes

Notes

Light testing has been done and core functionality is guaranteed, however more thorough testing is required.

akiraveliara · 2023-11-12T20:57:53Z

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 UseCommandsNext and UseSlashCommands as obsolete, for removal sometime in the v5 cycle; either in 5.0 if it takes a while, or in a following minor release.

OoLunar · 2023-11-12T21:01:11Z

Because the rest of DSharpPlus is on .NET 7, I've chosen to keep CNext and Slashies not obsolete until .NET 8 releases

akiraveliara · 2023-11-12T21:01:34Z

that'll be in a few days, and probably before merging this PR

OoLunar · 2023-11-12T21:03:21Z

Done

inftord · 2023-11-13T12:37:12Z

Before any documentation work happens, i would suggest to archive old command articles, instead of removing them completely

akiraveliara

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 #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/AbstractContext.cs

DSharpPlus.Commands/CommandExecutor.cs

DSharpPlus.Commands/Processors/TextCommands/Parsing/DefaultTextArgumentSplicer.cs

DSharpPlus.Commands/Processors/TextCommands/TextCommandProcessor.cs

DSharpPlus.Commands/Processors/TextCommands/TextLogging.cs

Directory.Packages.props

DSharpPlus.Commands/Commands/Attributes/ArgumentAttribute.cs

DSharpPlus.Commands/CommandsExtension.cs

DSharpPlus.Commands/Processors/SlashCommands/SlashCommandProcessor.cs

DSharpPlus.Commands/Processors/BaseCommandProcessor/ICommandProcessor.cs

DSharpPlus.Commands/Processors/TextCommands/TextCommandProcessor.cs

DSharpPlus.Commands/Converters/ByteConverter.cs

DSharpPlus.Commands/Converters/DateTimeConverter.cs

DSharpPlus.Commands/Converters/DiscordChannelConverter.cs

DSharpPlus.Commands/Converters/DiscordThreadChannelConverter.cs

OoLunar · 2023-11-13T22:27:41Z

DSharpPlus.Commands/Converters/IArgumentConverter.cs

+
+public interface IArgumentConverter<TEventArgs, TOutput> : IArgumentConverter where TEventArgs : AsyncEventArgs
+{
+    public Task<Optional<TOutput>> ConvertAsync(ConverterContext context, TEventArgs eventArgs);


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?

OoLunar · 2023-11-14T05:02:40Z

what exactly is the DX for configuring different processors? this seems a little doubtful given the implementation, but maybe you have a solution

What do you mean by DX?

akiraveliara · 2023-11-14T21:45:41Z

developer experience, as in, how tedious is it to configure a command processor

.github/workflows/build-commit.yml

DSharpPlus.Commands/Converters/DiscordMemberConverter.cs

DSharpPlus.Commands/Converters/DiscordMessageConverter.cs

Plerx2493 · 2023-11-14T22:35:26Z

DSharpPlus.Commands/Converters/DiscordUserConverter.cs

+            }
+        }
+
+        // If we didn't find the user in the guild, try to get the user from the API.


I dont like making hidden calls in converters maybe we can introduce a option to configure if we should make calls here

Yeah we can do that

Plerx2493 · 2023-11-14T23:05:47Z

DSharpPlus.Commands/CommandsExtension.cs

+    {
+        add
+        {
+            if (this.UseDefaultCommandErrorHandler)


I think we should toggle this.UseDefaultCommandErrorHandler when we unregister it

Plerx2493 · 2023-11-14T23:07:54Z

DSharpPlus.Commands/CommandsExtension.cs

+            this._commandErrored.Register(DefaultCommandErrorHandlerAsync);
+        }
+
+        // Attempt to get the user defined logging, otherwise setup a null logger since the D#+ Default Logger is internal.


I think we should open internals when merging

we should generally revamp that logger

DSharpPlus.Commands/ExtensionMethods.cs

Plerx2493 · 2023-11-14T23:23:51Z

DSharpPlus.Commands/Processors/SlashCommands/SlashCommandProcessor.cs

+    }
+
+    [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)


Maybe a bit more describtive parameter name.

It's... the parameter you're converting to an application parameter. How much more descriptive could it get?

I was talking about the last parameter, its confusing when you read a long method and then theres a random i

OH. Yeah I can do that

DSharpPlus.Commands/Processors/TextCommands/Parsing/DefaultTextArgumentSplicer.cs

…ong as all extra parameters have default values

…checks for message content; warn when missing required intents

OoLunar · 2024-01-30T15:39:07Z

All changes and review comments will be moved into issues as we need to start receiving user feedback soon. Merging into nightlies.

akiraveliara · 2024-01-30T15:40:19Z

i'll take a cursory look for intolerable flaws and then sign off on this

akiraveliara · 2024-01-30T15:44:23Z

inftord

i will probably make a separate issue/pr about xmldocs, and gonna write some of those myself.
other than that, lgtm!

* Oops! I sneezed. * One of you hooligans needs to fix your IDE formatting. `dotnet format` * Fix context menu processors requiring exact types instead of ish types * ConfigureCommands event from #1680 * Documentation; Changed equality implementation on `ConverterDelegateFactory`; Fixed runtime startup exceptions * Move `GetConverterFriendlyBaseType` to `IArgumentConverter` to prevent static implementation on every command processor * Several bug fixes to argument parsing and conversion logic - Finish moving `GetConverterFriendlyBaseType` - Prevent `AddConverter` from throwing when the same converter is attempting to be added again - Early return on `ParseParametersAsync` when no parameters need to be parsed - Move default value logic from `ParseParametersAsync` to `ParseParameterAsync` - `ParseParameterAsync` now provides the value that failed to parse when returning `ArgumentFailedConversionValue` - `ExecuteConverterAsync<T>` now handles multi-argument parameters * Shadow `InteractionConverterContext.Argument` so `ConverterContext.Argument` can still be the raw value * Fix enum parsing for text commands, which now properly supports boxing (is it called boxing?) * There can only be one reply to a Discord message * Defer multi-argument parameter parsing to the base implementation * Theoretically add support for `TextMessageReplyAttribute` to most argument converters * Use `IArgumentConverter.GetConverterFriendlyBaseType` * Configure commands event args. This is non-negotiable. * Use `IArgumentConverter.GetConverterFriendlyBaseType]`; Defer multi-argument parameter parsing to the base implementation * `IArgumentConverter.GetConverterFriendlyBaseType`; Theoretically better support multi-argument parameters with slash registration * Update deps * Resolve 1 nullable warning * Fix logic within user command registration, no longer allowing it to be forever false * Remove old event registration logic * Expose `IServiceProvider` instead of pointlessly hiding it * Switch from `AsyncEventArgs` to `DiscordEventArgs` * Move converter result values to their own namespace instead of repeatedly redeclaring on generic types * Remove ConverterFriendlySearchFlags; Add new `EnumConverter<T>`; Use the generic version of EnumConverter for parameters. * Fix compiler errors * Fix params registering all parameters as required * Fix choice providers not registering; Correctly isolated slash command logic away from the command parameter builder; Add an autocomplete provider for enums with more than 25 fields * Remove object constructors and replace them with more type-specific constructors for compile-time safety. * Vast improvements to the autocomplete and choice providers API - `SlashAutoCompleteProviderAttribute.AutoCompleteAsync` and `SlashChoiceProviderAttribute.GrabChoicesAsync` will now truncate when more than 25 elements are provided. - When options are truncated, it will log which source gave too many options, which it previous didn't. - When the type cannot be created, it will log which type failed along with the exception. - Try to use the numeric values for enums when possible, serializing only to `string` when the base type is `ulong`. - Support double and float enums, because apparently that's a thing? - Change the method signatures to return an enumerable of the object directly instead of dictionaries - Remove `SnakeCasedNameAttribute` in it's entirety, replacing it with `IParameterNamingPolicy`. * Fix compiler error * Expose the conversion result to `ArgumentParseException`; Documentation on related members. * Missed a spot * Fields to properties * Correctly apply limit * Just git- I mean get * Line feeds? * `git diff --name-only --diff-filter=AM master | grep ".cs" | xargs -I{line} echo \'{line}\' | xargs dotnet csharpier` * Update deps * Add generic overloads * Format code in docs * Grammar; New empty `T : new()` generic restraint on `AddConverter<T>()` * Update autocomplete/choice provider documentation * Update the missing slash commands section * Rename `DSharpPlus.Commands.Processors.SlashCommands.ParameterNamingPolicies` to `DSharpPlus.Commands.Processors.SlashCommands.NamingPolicies` * New `Naming Policies` article * Document multi-argument parameters * Don't throw when enums fail to convert * Support all time units from nanosecond to years * Give converters their own section; Briefly go over built-in argument converters and their quirks * Add a guide for invoking argument converters manually * Enforce max argument count in MAPs * Undo formatting from `dotnet csharpier` The new formatting now follows two very simply rules: - If it does not go beyond the end of my 1080p screen with the file bar open, it stays inlined. - If Roslyn didn't yell at me about it. * Allow 3rd party command processors to implement their own generic enum converter * Begone MAPs, welcome in VAPs * Remove raw numbers :( * Footnote * Formatting * #1362 was never merged * Throw away the rest of the MAPs * docs * Interface checks * Variadic * Attempt to resolve #1950 * Rename `ParameterNamingPolicy` to `NamingPolicy`; Fix inverted if statement * Use `GetCultureInfo` * More docs * Localizing interactions * Double space * Fix final GitHub review

OoLunar added enhancement help wanted core new-features on-hold docs big-change dependencies labels Nov 12, 2023

OoLunar added this to the v5.0 milestone Nov 12, 2023

OoLunar requested review from Plerx2493, VelvetToroyashi and akiraveliara November 12, 2023 19:57

OoLunar self-assigned this Nov 12, 2023

OoLunar changed the title ~~Add new extension: CommandAll~~ Add new extension: DSharpPlus.Commands Nov 12, 2023

akiraveliara requested changes Nov 13, 2023

View reviewed changes

inftord requested changes Nov 13, 2023

View reviewed changes

DSharpPlus.Commands/Commands/Attributes/ArgumentAttribute.cs Show resolved Hide resolved

inftord reviewed Nov 13, 2023

View reviewed changes

DSharpPlus.Commands/CommandsExtension.cs Outdated Show resolved Hide resolved

inftord reviewed Nov 13, 2023

View reviewed changes

DSharpPlus.Commands/CommandsExtension.cs Show resolved Hide resolved

inftord reviewed Nov 13, 2023

View reviewed changes

DSharpPlus.Commands/Processors/SlashCommands/SlashCommandProcessor.cs Outdated Show resolved Hide resolved

OoLunar commented Nov 13, 2023

View reviewed changes

DSharpPlus.Commands/Processors/BaseCommandProcessor/ICommandProcessor.cs Outdated Show resolved Hide resolved

DSharpPlus.Commands/Processors/TextCommands/TextCommandProcessor.cs Show resolved Hide resolved

OoLunar commented Nov 13, 2023

View reviewed changes

Plerx2493 reviewed Nov 14, 2023

View reviewed changes

OoLunar mentioned this pull request Nov 15, 2023

[ci-skip] Refactor CI #1682

Merged

OoLunar added 9 commits January 30, 2024 08:39

Argument length check

d80f356

Attempt to grab from default values when provided values aren't enough

7dbe4f4

Register message and user context commands with extra parameters as l…

f06d68d

…ong as all extra parameters have default values

Rename ExecuteConvertAsync to ExecuteConverterAsync

ec6299c

Resolve CA1859

ed9b40f

Default commands for groups

ad3a9b3

Add DSharpPlus.Commands to the DSharpPlus.sln

d808c5b

Resolve obsolete warnings

87ea23f

Configure newly added processors immediately; Add null or whitespace …

0504096

…checks for message content; warn when missing required intents

OoLunar force-pushed the master branch from 2ab4a0c to 0504096 Compare January 30, 2024 15:34

OoLunar marked this pull request as ready for review January 30, 2024 15:37

akiraveliara self-requested a review January 30, 2024 15:44

akiraveliara approved these changes Jan 30, 2024

View reviewed changes

inftord approved these changes Jan 30, 2024

View reviewed changes

OoLunar merged commit 8f0f9a8 into DSharpPlus:master Jan 30, 2024

OoLunar mentioned this pull request Jan 30, 2024

Introducing a new command framework #1553

Closed

VelvetToroyashi mentioned this pull request Jan 31, 2024

fix(commands): Fix slash command mapping #1729

Merged

OoLunar removed help wanted on-hold labels Feb 27, 2024

OoLunar added a commit that referenced this pull request Sep 25, 2024

ConfigureCommands event from #1680

df87e5c

OoLunar added a commit that referenced this pull request Oct 3, 2024

ConfigureCommands event from #1680

2071a23

OoLunar added a commit that referenced this pull request Oct 3, 2024

ConfigureCommands event from #1680

df4db72

OoLunar added a commit that referenced this pull request Oct 3, 2024

ConfigureCommands event from #1680

cacfd5d

OoLunar added a commit that referenced this pull request Oct 3, 2024

ConfigureCommands event from #1680

d83cccb

akiraveliara mentioned this pull request Nov 26, 2024

2025 Commands Rework #2179

Open

20 tasks

Uh oh!

Add new extension: DSharpPlus.Commands #1680

Add new extension: DSharpPlus.Commands #1680

Uh oh!

Conversation

OoLunar commented Nov 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Requirements

Roadmap

Notes

Uh oh!

akiraveliara commented Nov 12, 2023

Uh oh!

OoLunar commented Nov 12, 2023

Uh oh!

akiraveliara commented Nov 12, 2023

Uh oh!

OoLunar commented Nov 12, 2023

Uh oh!

inftord commented Nov 13, 2023

Uh oh!

akiraveliara left a comment • edited by OoLunar Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OoLunar commented Nov 14, 2023

Uh oh!

akiraveliara commented Nov 14, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Plerx2493 Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

OoLunar commented Jan 30, 2024

Uh oh!

OoLunar commented Nov 12, 2023 •

edited

Loading

akiraveliara left a comment •

edited by OoLunar

Loading

Plerx2493 Nov 14, 2023 •

edited

Loading