Skip to content
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

Change extension name sentry context to a mapping #295

Merged

Conversation

Galarzaa90
Copy link
Contributor

@Galarzaa90 Galarzaa90 commented May 10, 2024

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
Copy link
Member

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
Copy link
Contributor Author

Galarzaa90 commented May 29, 2024

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?

- Also add group and parent commands to Slash commands
@gdude2002
Copy link
Member

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
Copy link
Contributor Author

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
gdude2002 previously approved these changes Jun 4, 2024
@gdude2002
Copy link
Member

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
Copy link
Contributor Author

Galarzaa90 commented Jun 6, 2024

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
Copy link
Member

Looks good, thanks!

@gdude2002 gdude2002 merged commit e3da4b8 into Kord-Extensions:root Jun 7, 2024
3 checks passed
@Galarzaa90 Galarzaa90 deleted the bugfix/sentry-extension-context branch June 7, 2024 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants