Introducing a new command framework #1553

Instellate · 2023-05-11T20:11:39Z

Summary

Introduces a new command framework called UnifiedCommands.

Details

UnifiedCommands is a command framework that does not depend on old, code that is hard to maintain and features that should be removed. Meaning a fresh slate for everything.

UnifiedCommands also tries to simplify the process of bot development by going for a ASP.NET like approach making it faster to write methods for commands.

It also has pre execution in form of conditions which is run before a command gets executed to make sure that the event follows the condition needed for execution to proceed. This can also modify values given through Dependency Injection if it is a singleton or scope.

Note

This is still in WIP. If you have any questions. Please ask me in the discord server or comment in this pr.
I will mark this as ready when it's ready for production usage, all features are completed, and if this command framework will get accepted.

This doesn't include long, float and etc.

Also added DSharpPlus.CH into the sln and updated README.md

Also did some renaming on some of the proprties.

Did some renaming as well.

Also implemented a permission middleware for example. Some bug fixing is also present.

Updated TODO.md

horror, part 1

Also some roslyn analysing fixing.

This needs more work. Also added more to the TODO.md

Just need to make parameter mapping, conversion and binding.

This does not include error handling for it. Will do later.

I also did some bug fixing that I noticed in the code.

Also fixed so nullable is possible.

Also made so " " can be used in options as I forgot about that.

OoLunar · 2023-05-14T16:32:24Z

I'll think about it

OoLunar

This is the most thorough I've been on a PR in a long while. I hold nothing against you. I am not mad at you. I appreciate all the work you've put into this.

With that being said, I have a ton of grievances with this, mostly excluding formatting, see comments below. Additionally, I still don't understand what purpose this extension would serve over the existing CommandsNext and SlashCommands extensions.

OoLunar · 2023-05-15T17:01:34Z

DSharpPlus.UnifiedCommands/DiscordClientExtensionMethods.cs

@@ -0,0 +1,16 @@
+namespace DSharpPlus.UnifiedCommands;


Reference this:
https://github.com/OoLunar/DSharpPlus.CommandAll/blob/master/src/ExtensionMethods.cs

OoLunar · 2023-05-16T01:43:53Z

Directory.Packages.props

 <Project>
  <ItemGroup>
    <PackageVersion Include="Microsoft.CodeAnalysis.CSharp.Scripting" Version="4.6.0-1.final" />
+    <PackageVersion Include="Microsoft.Extensions.Configuration" Version="7.0.0" />


I am against adding MSE.Configuration as it's usually the developer which configures this dependency. Instead we should stick to using our own Configuration classes which the developer can configure themselves.

I didn't mean to have it like a configuration where you configure the class. I just following ASP.NET patterns a bit. But I will remove it to decrease dependencies as the use case is pretty useless.

.editorconfig

tools/AutoUpdateChannelDescription/DSharpPlus.Tools.AutoUpdateChannelDescription.csproj

OoLunar · 2023-05-16T01:47:39Z

DSharpPlus.Test/TestBot.cs

        this.Discord.UseInteractivity(icfg);

-        this.SlashCommandService = this.Discord.UseSlashCommands();
+        /* this.SlashCommandService = this.Discord.UseSlashCommands();


Why are we commenting out the slash commands tests?

I commented it out when testing the application part of the framework as SlashCommands would prevent me from testing that part. I haven't commented it out yet as I am still working on the application part.

What would be an alternative solution?

An alternative solution could be adding an option into the config.json for toggling on/off UnifiedCommands and SlashCommands.

DSharpPlus.UnifiedCommands/Application/Internals/ApplicationHandler.cs

OoLunar · 2023-05-16T02:55:28Z

DSharpPlus.UnifiedCommands/Application/Internals/ApplicationMethodParameterData.cs

+    public bool IsNullable { get; set; } = false;
+
+    public ApplicationMethodParameterData(string name)
+    => Name = name;


OoLunar · 2023-05-16T02:55:36Z

DSharpPlus.UnifiedCommands/Application/Internals/ApplicationModuleData.cs

+    public ObjectFactory Factory { get; }
+
+    public ApplicationModuleData(ObjectFactory factory)
+        => Factory = factory;


don't inline those please, having it indented by one level is fine - these lines can become very long, and id rather have all cases wrapped than some

OoLunar · 2023-05-16T02:56:11Z

DSharpPlus.UnifiedCommands/Application/ApplicationNameAttribute.cs

+{
+    public string Name { get; }
+    public string Description { get; }
+    // public Type Type { get; }


Why is this commented out and what function does it serve? Can we use generic attributes for it?

OoLunar

This is the most thorough I've been on a PR in a long while. I hold nothing against you. I am not mad at you. I appreciate all the work you've put into this.

With that being said, I have a ton of grievances with this, mostly excluding formatting, see comments below. Additionally, I still don't understand what purpose this extension would serve over the existing CommandsNext and SlashCommands extensions.

Instellate · 2023-05-16T11:31:13Z

I (was going to) initially started this as a personal project to have a more ASP.NET like experience with programing discord bots with D#+. As I don't like property/field di and I want proper implementation of scopes. And some other minor stuff. Talked about it a bit and it caught attention from @akiraveliara and @inftord.

Namely this is just trying to be a more "modern" and easier to use/convenient command framework. Therefor following a

public IApplicationResult Command() 
{
    return Reply("Hello there");
}

kind of way as that's how ASP.NET does their stuff.
Aki complained also that CNext is hard to maintain. So this is trying to be easy to maintain on top of that.

Plerx2493

I added some comments about logging exceptions to the user. Feel free to correct me if anything is wrong

Plerx2493 · 2023-05-16T15:09:15Z

DSharpPlus.UnifiedCommands/Application/Internals/ApplicationHandler.cs

+        {
+            _client.Logger.LogError(
+                e,
+                "Exception was thrown while trying to execute command {MethodName}. Was thrown from ",


Is this formatting right? looks as if there is something missing at the end. AFAIK the logger appends the exception which will be worded weird.:

Exception was thrown while trying to execute command {MethodName}. Was thrown from System.Exception ....

Plerx2493 · 2023-05-16T15:33:00Z

DSharpPlus.UnifiedCommands/Internals/CommandModuleRegister.cs

+                            if (parameter.ParameterType != typeof(string))
+                            {
+                                throw new InvalidMessageModuleStructure(
+                                    "You need to use string if you are using RemainingTextAttribute.");


Would be nice if the error message contains the methodname, so the user can see easily where they have to change something

Plerx2493 · 2023-05-16T15:33:13Z

DSharpPlus.UnifiedCommands/Internals/CommandModuleRegister.cs

+                        parameterData.Type = parameterType == typeof(double)
+                            ? MessageParameterDataType.Double
+                            : throw new InvalidMessageModuleStructure(
+                                                    $"You cannot use type {parameterType.Name} as a parameter.");


Would be nice if the error message contains the methodname, so the user can see easily where they have to change something

Plerx2493 · 2023-05-16T15:33:34Z

DSharpPlus.UnifiedCommands/Internals/CommandModuleRegister.cs

+                    ApplicationOptionAttribute? name = parameter.GetCustomAttribute<ApplicationOptionAttribute>();
+                    if (name is null)
+                    {
+                        throw new Exception("Parameter needs to have `ApplicationNameAttribute` marked.");


Would be nice if the error message contains the methodname, so the user can see easily where they have to change something

Plerx2493 · 2023-05-16T15:33:54Z

DSharpPlus.UnifiedCommands/Internals/CommandModuleRegister.cs

+                    {
+                        data.Type = parameter.ParameterType == typeof(DiscordAttachment)
+                            ? ApplicationCommandOptionType.Attachment
+                            : throw new Exception("Not a valid parameter type.");


Would be nice if the error message contains the methodname, so the user can see easily where they have to change something

akiraveliara

a note about formatting: don't inline initializer blocks, and if you wrap a line because it gets too long, use allman formatting. the pull request in its entirety is also sorely missing xmldocs.

it should be considered to unify the command conditions as far as possible. one of the main pain points with slash commands and commandsnext today is that they more or less duplicate this code.

you should probably consider adding todo tags for currently unfinished implementation so we can use IDE tooling to fill them all out before releasing a stable version.

i would furthermore like to reiterate my comments about changing Application* to Interaction, as well as restoring MessageCommand to its full length

DSharpPlus.UnifiedCommands/Application/ApplicationResult.cs

DSharpPlus.UnifiedCommands/Application/Conditions/IApplicationCondition.cs

akiraveliara · 2023-05-16T15:20:57Z

DSharpPlus.UnifiedCommands/Application/Entities/DiscordModalBuilder.cs

@@ -0,0 +1,6 @@
+namespace DSharpPlus.UnifiedCommands.Application.Entities;
+
+public class DiscordModalBuilder


could be moved to corelib, once finished

akiraveliara · 2023-05-16T15:22:19Z

DSharpPlus.UnifiedCommands/Application/Internals/ApplicationFactory.cs

+    internal void AddCondition(Func<IServiceProvider, IApplicationCondition> func)
+        => _conditionBuilders.Add(func);
+
+    internal void ExecuteCommand(DiscordInteraction interaction, DiscordClient client)


this method will need large-scale future refactoring, just to note it here.

akiraveliara · 2023-05-16T15:52:05Z

DSharpPlus.UnifiedCommands/Message/InvalidMessageConversionErrorType.cs

+/// <summary>
+/// Enum representing the possibilities for conversion failure.
+/// </summary>
+public enum InvalidMessageConversionType


on second thoughts yeah this should be changed to something allowing users to define their own failure types

akiraveliara · 2023-05-16T15:53:18Z

DSharpPlus.UnifiedCommands/Message/MessageOptionAttribute.cs

+public class MessageOptionAttribute : Attribute
+{
+    public string Option { get; set; }
+    public string? ShorthandOption { get; set; }


there might be merit in turning this into a char

Some shorthands might be more than one character. But usually that isn't the case. But I think we should keep shorthand option as a string for the people that needs it to be more than one character.

this does lock us out of combining shorthands à la git commit -aS

akiraveliara · 2023-05-16T15:54:45Z

DSharpPlus.UnifiedCommands/TODO.md

+
+## Decisions
+- Should we continue using DSharpPlus.Interactivity or should it be reimplemented into CommandModule?
+- Should we call classes as `xCommand` (for example `MessageCommand`) or should we call it `x` (for example `Message`) 


personally i think it should be Interaction and MessageCommand/TextCommand/whatever, our priority isn't with making names short

DSharpPlus.UnifiedCommands/UnifiedCommandsBuilder.cs

akiraveliara · 2023-05-16T15:55:56Z

DSharpPlus.UnifiedCommands/UnifiedCommandsBuilder.cs

+        Services.AddSingleton(configuration);
+        if (!Services.Any(s => s.ServiceType == typeof(IErrorHandler)))
+        {
+            client.Logger.LogTrace("Didn't find a error handler, using the default error handler");


probably LogDebug here

Why didn't I build test before pushing these times? I have no single clue.

…to a more dependency like system.

This now makes so converters aren't hard coded anymore.

Fixed a bug with shorthand options and also tree traversal

dongle-the-gadget · 2023-08-13T03:20:40Z

Does the new condition API allow dependency injection? I currently have a check that requires access to a DbContext in DI and I would like to use a single scope so that the DbContext can be reused for both the condition and the method.

OoLunar · 2024-01-30T16:16:55Z

Closed by #1680.

Instellate and others added 30 commits April 24, 2023 21:38

Started on the command handler.

9102db1

Merge remote-tracking branch 'origin/master'

6578bd8

Moved message related stuff into it's own namspace

531f1aa

Finished implementing all possible option types.

50e36c5

This doesn't include long, float and etc.

Some preperation for further commits.

f3b833b

Added README.md and TODO.md

33fbc0b

Added another TODO item.

2782a05

Also added DSharpPlus.CH into the sln and updated README.md

Started on the convertion error handler.

224b177

Also did some renaming on some of the proprties.

Changed a comment.

0b73af1

Implemented a way for developers to add convertion fail handling.

12efcaf

Added the option for DiscordRole.

f0c0a12

Did some renaming as well.

Implemented middlewares.

bca6173

Also implemented a permission middleware for example. Some bug fixing is also present.

Merge branch 'DSharpPlus:master' into master

6209ede

Tests is in DSharpPlus.Test

45500c2

Updated TODO.md

horror, part 1

8a4a802

Merge pull request #1 from InFTord/master

20ecd83

horror, part 1

Replaced every complain about Count().

48b40d6

Remade the Middleware system to only be pre module

f98cf4e

Also some roslyn analysing fixing.

Started implementing groups for commands.

dc8f90f

This needs more work. Also added more to the TODO.md

Started the optimisation of the option parser.

3303118

Optimised the parser.

93d4ab3

Just need to make parameter mapping, conversion and binding.

horror, part 2

ae0be7a

Completed the new routine system.

1f55ed2

This does not include error handling for it. Will do later.

Added names.md and updated README.md

2b16c19

Implemented error handling for invalid options into the new routine.

a59abb0

Fully implemented spacing in module and command names.

17fb223

I also did some bug fixing that I noticed in the code.

Added more TODO items.

80bb2c4

Added the possibilities for string options to have spaces by quotes.

f3c0361

Also fixed so nullable is possible.

Implemented default type.

1c87801

Also made so " " can be used in options as I forgot about that.

Merge branch 'DSharpPlus:master' into master

98895a7

OoLunar added new-features on-hold dependencies labels May 16, 2023

OoLunar suggested changes May 16, 2023

View reviewed changes

Instellate and others added 2 commits May 16, 2023 17:03

Addressed a big chunk of the reviews addressed.

aab7c7d

Merge branch 'DSharpPlus:master' into master

5af8730

Plerx2493 reviewed May 16, 2023

View reviewed changes

akiraveliara requested changes May 16, 2023

View reviewed changes

Instellate and others added 13 commits May 17, 2023 15:49

Merge branch 'DSharpPlus:master' into master

4741d1a

Addressed many of aki's review.

3b294c3

Added a missing semicolon.

8dddd38

Fixed all the building errors.

ccfeba6

Why didn't I build test before pushing these times? I have no single clue.

Merge branch 'DSharpPlus:master' into master

4553528

Started on IMessageConverter and remade the application conditions in…

395f631

…to a more dependency like system.

Merge branch 'master' into master

3c68abd

Added converters

3b6ebf8

This now makes so converters aren't hard coded anymore.

Started on adding overloads

bf0c985

Merge branch 'DSharpPlus:master' into master

42c2df9

Refactored error handler

de17425

Fixed a bug with shorthand options and also tree traversal

Merge branch 'DSharpPlus:master' into master

4bd084b

Merge remote-tracking branch 'origin/master'

c3b4c83

akiraveliara mentioned this pull request Jun 22, 2023

CommandsNext checking if user is a bot isn't optional #1369

Closed

akiraveliara removed the v5.x label Oct 5, 2023

akiraveliara mentioned this pull request Nov 13, 2023

Add new extension: DSharpPlus.Commands #1680

Merged

5 tasks

OoLunar closed this Jan 30, 2024

		@@ -0,0 +1,6 @@
		namespace DSharpPlus.UnifiedCommands.Application.Entities;

		public class DiscordModalBuilder

Uh oh!

Introducing a new command framework #1553

Introducing a new command framework #1553

Uh oh!

Conversation

Instellate commented May 11, 2023

Summary

Details

Note

Uh oh!

OoLunar commented May 14, 2023

Uh oh!

OoLunar left a comment

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!

Instellate May 17, 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!

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!

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!

OoLunar left a comment

Choose a reason for hiding this comment

Uh oh!

Instellate commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Plerx2493 left a comment

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!

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!

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!

Instellate May 17, 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!

Instellate May 17, 2023 •

edited

Loading

Instellate commented May 16, 2023 •

edited

Loading

Instellate May 17, 2023 •

edited

Loading