feat(mention): add mention for commands

[cptpiepmatz]

I realized that the mention crate is missing Display implementations for commands. Commands are formatted like this:

• </NAME:COMMAND_ID> for commands
• </NAME SUBCOMMAND:ID> for subcommands
• </NAME SUBCOMMAND_GROUP SUBCOMMAND:ID> for subcommand groups

This is documented here and in the changelog.

For the Mention implementation I checked what could be useful implementations. The tuples here are for convenience but maybe obsolete.

I considered implementing Mention for model::application::command::Command and model::application::application_command::CommandData but both them may contain mutiple subcommands or subcommand groups, therefore they are not feasible to implement.

Also the implementation needs not only the command id but also name (and maybe also subcommand and subcommand group). That's why I added the new enum. Three structs could also solve this but this was my personal preference.

[laralove143]

thank you for this catch

[cptpiepmatz]

I'm personally not quite sure whether to go with three structs and put them into the enum or just remove the implementations on the tuples.

I think three more structs would be a too much bloat, so I'm considering simply removing the tuple impls. What do you think @laralove143?

[laralove143]

i agree, the enum and the impl on command are probably enough

[cptpiepmatz]

Ok, I removed the tuple implementations and ordered the fields of the enum variants.

[cptpiepmatz]

While fixing the tests I realized that you have to wrap CommandMention inside MentionFormat manually, should I make a Mention implementation for CommandMention or directly implement Display for CommandMention?

@laralove143

[laralove143]

While fixing the tests I realized that you have to wrap CommandMention inside MentionFormat manually, should I make a Mention implementation for CommandMention or directly implement Display for CommandMention?

@laralove143

yes please, since thats how it works for other types

also we should have ParseMention for it as well

[cptpiepmatz]

yes please, since thats how it works for other types

I guess you mean we should have a implementation for Mention for CommandMention, right?

[cptpiepmatz]

@laralove143 The impl for Mention for CommandMention would need a clone:

impl Mention<CommandMention> for CommandMention {
    fn mention(&self) -> MentionFormat<CommandMention> {
        MentionFormat(self.clone())
    }
}

Should we keep that?

[laralove143]

@laralove143 The impl for Mention for CommandMention would need a clone:

impl Mention<CommandMention> for CommandMention {
    fn mention(&self) -> MentionFormat<CommandMention> {
        MentionFormat(self.clone())
    }
}

Should we keep that?

yes, as the alternative is to add a method to Mention that consumes self or take a &CommandMention, i think cloning small strings is negligible

[cptpiepmatz]

yes, as the alternative is to add a method to Mention that consumes self or take a &CommandMention, i think cloning small strings is negligible

I ended up doing a bit of both. This way, users can avoid the cloning if they prefer. The inner field of MentionFormat is private, so we needed to expose something to allow for this. I implemented Mention for CommandMention and noted in the documentation that this method uses clone. I also added the into_mention function that takes self and creates a MentionFormat<CommandMention> without any cloning.

also we should have ParseMention for it as well

I took care of this in commit 808b1c0. I also added a new variant to ParseMentionErrorType for cases where too many segments are found. The parse_mention method didn't really fit because it has a different pattern, and I didn't want to alter it, so the parse method is a bit longer than usual.

By the way, I saw that we have a MentionType enum. I was considering adding CommandMention to it, but that would mean it's no longer Copy. What do you think we should do here, @laralove143? Making CommandMention Copy doesn't feel right to me, as it's not as simple as cloning an Id.

[laralove143]

thank you for the changes, theres only a few nits remaining

[laralove143]

about MentionType, it wouldnt make a difference in practice, it'd still use Copy for the other variants, so i think its okay to add the new variant there

[cptpiepmatz]

about MentionType, it wouldnt make a difference in practice, it'd still use Copy for the other variants, so i think its okay to add the new variant there

You cannot derive Copy as CommandMention isn't Copy. And I don't think that CommandMention should be Copy. I would be fine to manually implement Copy for a MentionType with a Command variant.

[laralove143]

about MentionType, it wouldnt make a difference in practice, it'd still use Copy for the other variants, so i think its okay to add the new variant there

You cannot derive Copy as CommandMention isn't Copy. And I don't think that CommandMention should be Copy. I would be fine to manually implement Copy for a MentionType with a Command variant.

i think marking it Copy when it might not be copy is misleading but taking away Copy from MentionType would make this pr breaking, so i think we should open another pr for adding CommandMention to MentionType that targets next

[cptpiepmatz]

That sounds like the best approach to me. So is this done?

[laralove143]

thank you for the addition!

[AEnterprise]

looks good, just a small issue left in the docs build with a redundant import

[cptpiepmatz]

@AEnterprise I checked the logs of that failing build, it fails on:

twilight-http/src/json.rs:4:29

I never touched that file in my PR.

[cptpiepmatz]

So what do we do now?

[laralove143]

im sure JsonDeserializer is actually used, maybe not in conditional compiling (used under #[cfg(not(doc))]) but this is very unlikely, i think its a false positive, maybe because of incremental building (it is used but not in the changes in this pr) ill go ahead and merge this