Kord-Extensions / kord-extensions

Kord extensions framework, providing commands and distinct units of functionality
https://kordex.dev
European Union Public License 1.2
110 stars 27 forks source link

Change extension name sentry context to a mapping #295

Closed Galarzaa90 closed 3 months ago

Galarzaa90 commented 4 months ago

We discussed this briefly on Discord: https://canary.discord.com/channels/1121419906995458098/1210756178515206164

But basically, Sentry is expecting an object, not a single value, so it shows this warning:

image

This is ready as it is, but this change made me think of further possible changes:

  1. Do we want to have Sentry show another "section" for a single value, or should extension.name be merged into command as command.extension?.
    image
  2. Also noticing that for slash commands, only the command/subcommand's name is shown, and no information on parent command is shown ( the command in the above picture is actually /respawn-manage bump-user). So I'm thinking of doing either of these:
    1. Leave name as is, add parent if it is a subcommand and put the parent's name there.
    2. Make name always be the root command, and add subcommand if it is a subcommand? Probably a good idea to also have group in here.

I think having just the name would be ambiguous as it is common to have a pattern like: /guild edit, /member edit, /channel edit, so they would all be listed as "edit".

gdude2002 commented 4 months ago

Sorry, I've been pretty busy.

  1. I don't see why not merge it.
  2. I think parent would work best for slash commands. For chat commands, the tree can be infinitely long - I guess parents in that case.
Galarzaa90 commented 4 months ago

Alright, so I'm working in those changes now.

One question, I see that for chat commands we have translatedName, but for the rest we are using name which would be either the command's name or a translation key (e.g. command.banana-flat), is this desired?

In my opinion, I think it would be a good idea to use localizedName.default for all application commands. Translated names wouldn't be ideal, since in some cases, translations are done by other people, so the ones actually using sentry might not know what the command is from the translated name. Using name would be acceptable too, since it would be the same regardless of the actual language used in the call.

I haven't used chat commands, so I'm not sure what name would be here, would that always be the default name?

gdude2002 commented 4 months ago

We should use the name property in all cases, as it's the string provided in the code. It's up to the developer to make the translation key descriptive, but that should come naturally.

Galarzaa90 commented 4 months ago

This is ready for review. Only thing missing would be chat command parents, but I'm not familiar with chat commands. But name is already the full command in this case.

To be honest I would lean towards doing the same in Slash commands, have name be the full command text, so it matches chat commands. /object edit prop -> name=object edit prop, instead of: name=prop, group=edit, parent=object.

If that would be preferred, I can make that change, otherwise, this is ready.

gdude2002 commented 3 months ago

I think splitting them out should aid with more complex filtering. That said, I'm unsure about the inconsistency there - though then again, I suppose combining them would more easily allow partial matches?

Ah, screw it, let's combine them. I'll merge after that.

Galarzaa90 commented 3 months ago

Merged them.

Example name key with test commands:

name=command.banana-group command.banana command.banana

Which corresponds to this:

publicSlashCommand {
    name = "command.banana-group"
    description = "Translated banana group"

    group("command.banana") {
        description = "Translated banana group"

        publicSubCommand {
            name = "command.banana"
            description = "Translated banana"
gdude2002 commented 3 months ago

Looks good, thanks!