Skip to content

Fix/v4/slashies-cleanup custom converter support #1382

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.

Sign up for GitHub

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

Jump to bottom
Merged
VelvetToroyashi merged 30 commits into from
Jan 28, 2023
Merged

Fix/v4/slashies-cleanup custom converter support #1382

VelvetToroyashi merged 30 commits into from
Jan 28, 2023

Conversation

@OoLunar
Copy link
Contributor

@OoLunar OoLunar commented Aug 29, 2022

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.

Officially passing #1362 onto @ExaInsanity

selfdocumentingcode and others added 13 commits July 28, 2022 10:49
@selfdocumentingcode
Change GuildMemberUpdatedEventArgs to contain before and after Discor…
b98f62a 
…dMember objects (#1359)
@waffle-lord
Fix #1325 - WebhookBuilder Throws Bad Request (#1363)
168dbf9 
* use DiscordMentions on RestWebhookPayloads

* keep null value when null

* remove null check from discordmentions arg
@OoLunar
Implemented PATCH /guilds/:guild_id/roles (tested) (#1311)
254ba1d
@OoLunar
Fix compile errors caused by #1333
570040e
@OoLunar
Update hosting.md
e2bfeef
@OoLunar
Remove unused DSharpPlus.MSTest folder
2bcabe3
@OoLunar
Cache threads upon thread creation (#1366)
f85c91f
@akiraveliara
categorization for the cnext default help command (#1371)
00530ed
@selfdocumentingcode @OoLunar
Fix memberBefore missing roles in GuildMemberUpdateEventArgs when not… (
8447942 
#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>
@OoLunar
Ported CommandsNext cooldown attribute to Slash Commands (#1375)
eba2fba 
* Ported CommandsNext cooldown attribute to Slash Commands; Fixed compiler warnings and suggestions

* Still untested; Attempted to resolve @Erisa's review
@OoLunar
Made all exceptions inherit from ApplicationCommandExecutionChecksFai…
da296c0 
…ledException (previously Exception)
@OoLunar
Start on converters
c9b68f1
@OoLunar
Added custom converter support:
4fc014a 
- 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.
@CabbageAdi CabbageAdi changed the title Fix/v4/slashies cleanup Fix/v4/slashies-cleanup custom converter support Sep 10, 2022
@OoLunar
Copy link
Contributor Author

OoLunar commented Oct 27, 2022

I have done minimum testing with these changes and can confirm that my slash commands are correctly functioning with full built-in converter support. I have not tested external converter support, though in theory it should work as intended unless if the (un)register converter methods break.

OoLunar added a commit to OoLunar/Tomoe that referenced this pull request Oct 28, 2022
@OoLunar
Introduce DisMoonSharp to Tomoe
2f430d4 
For legal reasons, this is a joke.
- Fork DSharpPlus and make changes to include a public argument converter system until DSharpPlus/DSharpPlus#1382 and https://github.com/DSharpPlus/DSharpPlus/tree/fix/v4/slashies-cleanup are merged. This fork is not intended to be redistributed or maintained. Modification to this fork may only be done by me and only I may use this fork.
- Submodule my fork of DSharpPlus into libs/DSharpPlus
- Move `src/` to `Tomoe/src/` and add it to the newly created SLN file
- Remake `/raw` to use the new argument conversion system.
Naamloos
Naamloos reviewed Nov 1, 2022
View reviewed changes
Copy link
Member

@Naamloos Naamloos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now here is something interesting to consider:

Should an enum argument type also implement an autofill service on the library's end?

@OoLunar
Copy link
Contributor Author

OoLunar commented Nov 1, 2022

Assuming we're both referencing Slash Command Choices (the list of valid arguments defined by the bot), yes Enums should be handled differently and should register the choices. I believe the current slash commands extension does this and my changes would actually the break current behavior. I'll fix this

@OoLunar
Copy link
Contributor Author

OoLunar commented Nov 1, 2022

Latest commit has fixed choices not being registered from enums (tested). I also added number-like converters for sbyte, short, int along with their unsigned counterparts (ulong included)

@OoLunar
Copy link
Contributor Author

OoLunar commented Nov 2, 2022

ParamLimitAttribute is a temporary name. Tested my changes and confirmed working.

Naamloos
Naamloos approved these changes Nov 13, 2022
View reviewed changes
Copy link
Member

@Naamloos Naamloos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm I really like these changes

@OoLunar
Copy link
Contributor Author

OoLunar commented Nov 13, 2022 •
edited
Loading

I'll need someone to test most functionally beside top level slash commands and registering/using custom argument converters. I didn't make many changes but it's better to be sure

akiraveliara
akiraveliara reviewed Nov 17, 2022
View reviewed changes
Copy link
Member

@akiraveliara akiraveliara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now, for all of the integer converters: i'd prefer, in line with the (BCL)[https://learn.microsoft.com/en-us/dotnet/api/system.buffers.binary.binaryprimitives?view=net-7.0], to use framework names for the type they convert (citing one example here).

if not, i'd use the keywords, that is UnsignedInt -> UInt.

also: obligatory tested? does this work with existing slash command setups (specifically nullability)?

@OoLunar
Copy link
Contributor Author

OoLunar commented Nov 17, 2022

also: obligatory tested?

As commented above, no testing was done beyond top level slash commands and registering/using custom argument converters. Some BCL converters have been used (not all).

does this work with existing slash command setups (specifically nullability)?

Nullability should absolutely work.

@OoLunar
Copy link
Contributor Author

OoLunar commented Nov 17, 2022

@akiraveliara all of your comments should be resolved. Is there anything else I should fix before this gets merged into the fix/v4/slashies-cleanup branch?

akiraveliara
akiraveliara approved these changes Nov 17, 2022
View reviewed changes
Copy link
Member

@akiraveliara akiraveliara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, without testing yet

@VelvetToroyashi VelvetToroyashi merged commit b154811 into DSharpPlus:fix/v4/slashies-cleanup Jan 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Naamloos Naamloos Naamloos approved these changes

@VelvetToroyashi VelvetToroyashi VelvetToroyashi approved these changes

@akiraveliara akiraveliara akiraveliara approved these changes

Assignees

No one assigned

Labels

new-features obsolete: slash-commands

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@OoLunar @Naamloos @VelvetToroyashi @akiraveliara @CabbageAdi @selfdocumentingcode @waffle-lord