Commands Processor Cleanup #2075

OoLunar · Aug 18, 2024

Summary

When writing up #2042, @quinchs had helpfully pointed out that the event args generic constraint is not required at all for invoking argument converters - instead the relevant data should be extracted to the most specific converter context type and passed to the ConvertAsync delegate through abstract classes - very similar to how we currently do CommandContexts. This PR grew significantly larger than intended as it involved cleaning up the base command processor, extracting logic to other classes or merging duplicate code into virtual methods. Here's a generalized changelog of what's been done so far:

Notes

Tested. Only accepting feedback regarding grand design or possible functional bugs. Other feedback regarding docs, formatting or nits will be discarded - you have push access to this branch and I'd expect those final changes to be made before merging, not during active development.

Generated Changelog

This changelog does not account for renames and does include internal/private members.

New Types:

MultiArgumentAttribute
ArgumentFailedConversionResult
ArgumentNotParsedResult
ConfigureCommandsEventArgs
ConverterDelegateFactory
MessageCommandLogging
EnumAutoCompleteProvider
EnumOptionChoiceProvider
IInteractionNamingPolicy
KebabCaseNamingPolicy
LowercaseNamingPolicy
SnakeCaseNamingPolicy
UserCommandLogging

Removed Types:

SnakeCasedNameAttribute
ConverterSentinel
LazyConverter
EnumOptionProvider

Added Methods:

ValueTask CommandsExtension.BuildCommandsAsync();
Regex DiscordChannelConverter.GetChannelMatchingRegex();
ConverterDelegate ConverterDelegateFactory.CreateConverterDelegate();
void BaseCommandProcessor.AddEnumConverters();
ValueTask<IReadOnlyDictionary<CommandParameter, object?>> BaseCommandProcessor.ParseParametersAsync(TConverterContext converterContext);
ValueTask<object?> BaseCommandProcessor.ParseParameterAsync(TConverterContext converterContext);
string IInteractionNamingPolicy.GetCommandName(Command command);
string IInteractionNamingPolicy.GetParameterName(CommandParameter parameter, int arrayIndex);
StringBuilder IInteractionNamingPolicy.TransformText(ReadOnlySpan<char> text);
string KebabCaseNamingPolicy.GetCommandName(Command command);
string KebabCaseNamingPolicy.GetParameterName(CommandParameter parameter, int arrayIndex);
StringBuilder KebabCaseNamingPolicy.TransformText(ReadOnlySpan<char> text);
string LowercaseNamingPolicy.GetCommandName(Command command);
string LowercaseNamingPolicy.GetParameterName(CommandParameter parameter, int arrayIndex);
StringBuilder LowercaseNamingPolicy.TransformText(ReadOnlySpan<char> text);
string SnakeCaseNamingPolicy.GetCommandName(Command command);
string SnakeCaseNamingPolicy.GetParameterName(CommandParameter parameter, int arrayIndex);
StringBuilder SnakeCaseNamingPolicy.TransformText(ReadOnlySpan<char> text);
ValueTask SlashCommandProcessor.PopulateMultiArgumentParametersAsync(Command command, List<DiscordApplicationCommandOption> options);
Task SlashCommandProcessor.ConfigureCommands(CommandsExtension extension, ConfigureCommandsEventArgs eventArgs);
void TextConverterContext.SwitchToMessage(bool value);

Removed Methods:

void CommandsExtension.BuildCommands();
Regex DiscordChannelConverter.getChannelRegex();
Regex DiscordThreadChannelConverter.getChannelRegex();
ValueTask<TCommandContext?> BaseCommandProcessor.ParseArgumentsAsync(TConverterContext converterContext, TEventArgs eventArgs);
string SlashCommandProcessor.ToSnakeCase(ReadOnlySpan<char> str);
ValueTask<SlashCommandContext?> SlashCommandProcessor.ParseArgumentsAsync(InteractionConverterContext converterContext, InteractionCreatedEventArgs eventArgs);
ValueTask<TextCommandContext?> TextCommandProcessor.ParseArgumentsAsync(TextConverterContext converterContext, MessageCreatedEventArgs eventArgs);

Added Properties:

int MultiArgumentAttribute.MaximumArgumentCount { get; init; }
int MultiArgumentAttribute.MinimumArgumentCount { get; init; }
Exception? ArgumentFailedConversionResult.Error { get; init; }
List<CommandBuilder> ConfigureCommandsEventArgs.CommandTrees { get; init; }
BaseCommandProcessor<TConverter, TConverterContext, TCommandContext> ConverterDelegateFactory.CommandProcessor { get; init; }
IInteractionNamingPolicy InteractionConverterContext.ParameterNamePolicy { get; init; }
int InteractionConverterContext.ArgumentEnumerableIndex { get; init; }
IInteractionNamingPolicy SlashCommandConfiguration.ParameterNamePolicy { get; init; }
bool TextMessageReplyAttribute.RequiresReply { get; init; }
DiscordMessage TextConverterContext.Message { get; init; }
bool TextConverterContext.IsOnMessageReply { get; init; }

Removed Properties:

ConverterDelegate<TEventArgs>? LazyConverter.ConverterDelegate { get; init; }
CommandParameter AutoCompleteContext.AutoCompleteArgument { get; init; }
IReadOnlyDictionary<Type, DiscordApplicationCommandOptionType> SlashCommandProcessor.TypeMappings { get; init; }
bool TextMessageReplyAttribute.RequireReplies { get; init; }

Added Fields:

AsyncEvent<CommandsExtension, ConfigureCommandsEventArgs> CommandsExtension.configuringCommands;
Type EnumConverter.baseEnumType;
Action<ILogger, string, string, Exception?> BaseCommandLogging.invalidArgumentConverterImplementation;
Action<ILogger, string, string, string, string, Exception?> BaseCommandLogging.invalidEnumConverterImplementation;
Action<ILogger, string, string, string, Exception?> BaseCommandLogging.duplicateArgumentConvertersRegistered;
MethodInfo ConverterDelegateFactory.createConverterDelegateMethod;
ConverterDelegate? ConverterDelegateFactory.converterDelegate;
Dictionary<Type, ConverterDelegateFactory> BaseCommandProcessor.converterFactories;
Action<ILogger, Exception?> MessageCommandLogging.interactionReceivedBeforeConfigured;
Action<ILogger, string, Exception?> MessageCommandLogging.messageCommandCannotHaveSubcommands;
Action<ILogger, string, Exception?> MessageCommandLogging.messageCommandContextParameterType;
Action<ILogger, string, Exception?> MessageCommandLogging.invalidParameterType;
Action<ILogger, int, string, Exception?> MessageCommandLogging.invalidParameterMissingDefaultValue;
DiscordAutoCompleteChoice[] EnumAutoCompleteProvider.choices;
bool SlashCommandProcessor.isApplicationCommandsRegistered;
Action<ILogger, Exception?> SlashLogging.interactionReceivedBeforeConfigured;
string TextConverterContext.rawArguments;
DiscordMessage TextConverterContext.message;
TextMessageReplyAttribute? TextConverterContext.replyAttribute;
TextConverterContext? TextConverterContext.replyConverterContext;
Action<ILogger, DiscordIntents, Exception?> TextLogging.missingRequiredIntents;
Action<ILogger, Exception?> TextLogging.missingMessageContentIntent;
Action<ILogger, Exception?> UserCommandLogging.interactionReceivedBeforeConfigured;
Action<ILogger, string, Exception?> UserCommandLogging.userCommandCannotHaveSubcommands;
Action<ILogger, string, Exception?> UserCommandLogging.userCommandContextParameterType;
Action<ILogger, string, Exception?> UserCommandLogging.invalidParameterType;
Action<ILogger, int, string, Exception?> UserCommandLogging.invalidParameterMissingDefaultValue;

Removed Fields:

Action<ILogger, string, string, Exception?> BaseCommandLogging.InvalidArgumentConverterImplementation;
Action<ILogger, string, string, string, Exception?> BaseCommandLogging.DuplicateArgumentConvertersRegistered;
Dictionary<Type, LazyConverter> BaseCommandProcessor.lazyConverters;
bool SlashCommandProcessor.configured;
bool SlashCommandProcessor.registered;
bool TextCommandProcessor.configured;
Action<ILogger, DiscordIntents, Exception?> TextLogging.MissingRequiredIntents;
Action<ILogger, Exception?> TextLogging.MissingMessageContentIntent;

Added Events:

CommandsExtension.ConfiguringCommands

DSharpPlus.Commands/Converters/DiscordAttachmentConverter.cs

VelvetToroyashi

At an absolute bare glance, there isn't much that looks off - I do have questions and concerns, but with the vague notion of "only large design issues" wishing to be addressed, I've commented on anything abhorrently problematic.

akiraveliara

some functional issues to fix

re: formatting, that isn't how contributing to a project works at all, and while your IDE breaking is somewhat of an argument i do have grievances with going out and making the formatting worse to then tell other people to go and fix that. i do not think that's okay.

re pooling multi-argument parameter arrays, i took a brief look at that and it's not really worth it, these arrays are relatively small - it could be worth it for arrays of very large flat types (like making an Int512 type in flat memory), but for reference-type and primitive arrays it's not usually worth it, particularly not for slash commands

DSharpPlus.Commands/ArgumentModifiers/MultiArgumentAttribute.cs

DSharpPlus.Commands/Processors/SlashCommands/EnumAutoCompleteProvider.cs

DSharpPlus.Commands/Processors/SlashCommands/Localization/LocalesHelper.cs

Checking to see if I can rebase the PR or not. Rerequesting in a moment.

…actory`; Fixed runtime startup exceptions

…t static implementation on every command processor

- 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

akiraveliara · Oct 3, 2024

consider "variadic parameters" instead of "var-arg parameters" in docs - that's actually english :P

docs/articles/commands/var_args_parameters.md

…tement

docs/articles/commands/processors/slash_commands/translator_providers.md

docs/articles/commands/processors/text_commands/command_aliases.md

docs/articles/toc.yml

Resolved

* 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 bugfix enhancement big-change commands For issues related to DSharpPlus.Commands labels Aug 18, 2024

OoLunar added this to the v5.0 milestone Aug 18, 2024

OoLunar self-assigned this Aug 18, 2024

OoLunar force-pushed the oolunar/commands/converter-cleanup branch from 30cfcd8 to 7aeef2a Compare September 25, 2024 05:07

OoLunar marked this pull request as ready for review September 27, 2024 04:51

OoLunar requested review from Plerx2493, VelvetToroyashi and akiraveliara September 27, 2024 04:51

OoLunar added help wanted new-features docs dependencies labels Sep 27, 2024

VelvetToroyashi reviewed Sep 27, 2024

View reviewed changes

DSharpPlus.Commands/Converters/DiscordAttachmentConverter.cs Show resolved Hide resolved

VelvetToroyashi reviewed Sep 27, 2024

View reviewed changes

akiraveliara previously requested changes Sep 27, 2024

View reviewed changes

OoLunar requested a review from akiraveliara September 27, 2024 17:23

OoLunar force-pushed the oolunar/commands/converter-cleanup branch 3 times, most recently from ac48e92 to fe3e25e Compare October 3, 2024 04:18

OoLunar added 7 commits October 2, 2024 23:22

Oops! I sneezed.

ec62be4

One of you hooligans needs to fix your IDE formatting. dotnet format

44f7606

Fix context menu processors requiring exact types instead of ish types

b5603a7

ConfigureCommands event from #1680

d83cccb

Documentation; Changed equality implementation on `ConverterDelegateF…

083cbe8

…actory`; Fixed runtime startup exceptions

Move GetConverterFriendlyBaseType to IArgumentConverter to preven…

00a5ca4

…t static implementation on every command processor

Remove raw numbers :(

8cb633e

akiraveliara mentioned this pull request Oct 4, 2024

v5 mega-issue #1580

Open

36 tasks

OoLunar added 6 commits October 4, 2024 11:18

Footnote

ee9e5af

Formatting

06df84a

#1362 was never merged

1e21b6b

Throw away the rest of the MAPs

25ed516

docs

2c12c1b

Interface checks

b000d88

akiraveliara requested changes Oct 4, 2024

View reviewed changes

docs/articles/commands/var_args_parameters.md Show resolved Hide resolved

Variadic

3f08a63

akiraveliara approved these changes Oct 4, 2024

View reviewed changes

OoLunar added 4 commits October 4, 2024 16:00

Attempt to resolve #1950

867f25a

Rename ParameterNamingPolicy to NamingPolicy; Fix inverted if sta…

122740d

…tement

Use GetCultureInfo

694bde2

More docs

9ab8059

akiraveliara requested changes Oct 5, 2024

View reviewed changes

akiraveliara removed help wanted dependencies labels Oct 5, 2024

OoLunar added 2 commits October 5, 2024 23:56

Localizing interactions

9260fb0

Double space

08c5bf9

OoLunar requested a review from akiraveliara October 6, 2024 04:58

akiraveliara previously requested changes Oct 6, 2024

View reviewed changes

docs/articles/toc.yml Outdated Show resolved Hide resolved

Fix final GitHub review

6113d73

akiraveliara approved these changes Oct 6, 2024

View reviewed changes

akiraveliara merged commit b242007 into master Oct 6, 2024
1 check passed

OoLunar deleted the oolunar/commands/converter-cleanup branch October 7, 2024 17:04

Sella-GH mentioned this pull request Oct 25, 2024

Incompatibility of DSP.Commands with Invariant-Culture mode #2130

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Commands Processor Cleanup #2075

Commands Processor Cleanup #2075

Uh oh!

OoLunar commented Aug 18, 2024 •

edited by akiraveliara

Loading

Uh oh!

Uh oh!

VelvetToroyashi left a comment

Uh oh!

akiraveliara left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

akiraveliara commented Oct 3, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Search code, repositories, users, issues, pull requests...

Uh oh!

Commands Processor Cleanup #2075

Commands Processor Cleanup #2075

Uh oh!

Conversation

OoLunar commented Aug 18, 2024 • edited by akiraveliara Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Notes

Generated Changelog

New Types:

Removed Types:

Added Methods:

Removed Methods:

Added Properties:

Removed Properties:

Added Fields:

Removed Fields:

Added Events:

Uh oh!

Uh oh!

VelvetToroyashi left a comment

Choose a reason for hiding this comment

Uh oh!

akiraveliara left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

akiraveliara commented Oct 3, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

OoLunar commented Aug 18, 2024 •

edited by akiraveliara

Loading