Closed cptpiepmatz closed 12 months ago
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?
i agree, the enum and the impl on command are probably enough
Ok, I removed the tuple implementations and ordered the fields of the enum variants.
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
While fixing the tests I realized that you have to wrap
CommandMention
insideMentionFormat
manually, should I make aMention
implementation forCommandMention
or directly implementDisplay
forCommandMention
?@laralove143
yes please, since thats how it works for other types
also we should have ParseMention
for it as well
yes please, since thats how it works for other types
I guess you mean we should have a implementation for Mention
for CommandMention
, right?
@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 The impl for
Mention
forCommandMention
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
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 808b1c0720a958717534848b1265a7949d2d9eb9. 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
.
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
about
MentionType
, it wouldnt make a difference in practice, it'd still useCopy
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.
about
MentionType
, it wouldnt make a difference in practice, it'd still useCopy
for the other variants, so i think its okay to add the new variant thereYou cannot derive
Copy
asCommandMention
isn'tCopy
. And I don't think thatCommandMention
should beCopy
. I would be fine to manually implementCopy
for aMentionType
with aCommand
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
That sounds like the best approach to me. So is this done?
@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.
So what do we do now?
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
I realized that the
mention
crate is missingDisplay
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 groupsThis 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
formodel::application::command::Command
andmodel::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.