-
-
Notifications
You must be signed in to change notification settings - Fork 318
categorization for the cnext default help command #1371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
categorization for the cnext default help command #1371
Conversation
| namespace DSharpPlus.CommandsNext.Attributes | ||
| { | ||
| [AttributeUsage(AttributeTargets.Method, AllowMultiple = false, Inherited = true)] | ||
| public sealed class CategoryAttribute : Attribute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pain. I'd ask if we really need yet another attribute when one exists in the BCL, but we've rolled our own for everything else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, that was roughly my line of thought. would be strange to somehow need System.ComponentModel for just this one attribute and none other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep it consistent. I do think we should have a PR that replaces the CNext attributes with the ComponentModel attributes, but that'd be a big breaking change and should require an announcement
|
|
||
| foreach(var category in categories) | ||
| { | ||
| this.EmbedBuilder.AddField(category, string.Join(", ", subcommands.Where(xm => xm.Category == category).Select(xm => Formatter.InlineCode(xm.Name))), false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of constantly re-iterating over the entire command list, consider a GroupBy statement instead.
|
No description provided. |
OoLunar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I guess
| namespace DSharpPlus.CommandsNext.Attributes | ||
| { | ||
| [AttributeUsage(AttributeTargets.Method, AllowMultiple = false, Inherited = true)] | ||
| public sealed class CategoryAttribute : Attribute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep it consistent. I do think we should have a PR that replaces the CNext attributes with the ComponentModel attributes, but that'd be a big breaking change and should require an announcement
| commandBuilder.WithHiddenStatus(true); | ||
| break; | ||
|
|
||
| case CategoryAttribute c: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable naming >:(
| { | ||
| this.EmbedBuilder.AddField(this.Command is not null ? "Subcommands" : "Commands", string.Join(", ", subcommands.Select(x => Formatter.InlineCode(x.Name))), false); | ||
|
|
||
| var categories = subcommands.GroupBy(xm => xm.Category).OrderBy(xm => xm.Key == null).ThenBy(xm => xm.Key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surely this can be done differently, more efficiently?
* Change GuildMemberUpdatedEventArgs to contain before and after DiscordMember objects (#1359) * Fix #1325 - WebhookBuilder Throws Bad Request (#1363) * use DiscordMentions on RestWebhookPayloads * keep null value when null * remove null check from discordmentions arg * Implemented PATCH /guilds/:guild_id/roles (tested) (#1311) * Fix compile errors caused by #1333 * Update hosting.md * Remove unused DSharpPlus.MSTest folder * Cache threads upon thread creation (#1366) * categorization for the cnext default help command (#1371) * Fix memberBefore missing roles in GuildMemberUpdateEventArgs when not… (#1364) * Fix memberBefore missing roles in GuildMemberUpdateEventArgs when not in cache * Update cached guild member in OnGuildMemberUpdateEventAsync internal event handler * Update DiscordClient.Dispatch.cs Co-authored-by: Lunar Starstrum <lunar@forsaken-borders.net> * Ported CommandsNext cooldown attribute to Slash Commands (#1375) * Ported CommandsNext cooldown attribute to Slash Commands; Fixed compiler warnings and suggestions * Still untested; Attempted to resolve @Erisa's review * Made all exceptions inherit from ApplicationCommandExecutionChecksFailedException (previously Exception) * Start on converters * Added custom converter support: - No, I did not implement the checks in the converters. I felt that should be done in a separate area of code - Yes, I did break my Enter, Backspace, D and Apostrophe keys because of this PR. Don't ask how. * Fixed startup errors; Added DiscordMessageArgumentConverter; Added (un)register converter methods (untested). * Fix registering converters * (untested) Add number converters; Fix enums not registering choices correctly * Add DiscordMember converter; Switch from ArgumentException to new MissingArgumentConverterException * Actually fix Nullable and update TimeSpan converter to match with CNext * Add params/array support * Boundry checks * [ci-skip] Resolve naming errors * "One less variable allocation" * Remove unsupport NETSTANDARD1_3 support; Un-Emziware those variable names * Remove unused property * Extra parameter limit attribute check * Remove UnregisterConverter(s) methods * Changed BCL types to use framework names * Pass InnerException to prevent passing TargetInvocationException * Fix enum support (again) --------- Co-authored-by: Bob Loblaw <razvanmandache@gmail.com> Co-authored-by: waffle-lord <76401815+waffle-lord@users.noreply.github.com> Co-authored-by: Iris Féanorá <61018569+ExaInsanity@users.noreply.github.com>
as discussed approximately here: https://discord.com/channels/379378609942560770/379386901725052928/995957157298192447
this PR adds support for categorizing commands using a new
CategoryAttribute. the categorizing behaviour is only invoked when at least one command in the searched batch is marked with the attribute, and it can be disabled by commenting out all related attributes - i do not believe a config toggle makes sense here, given the configuration is already quite huge.