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

Reply edit without reset embed #300

Open
Morb0 opened this issue Aug 19, 2024 · 1 comment
Open

Reply edit without reset embed #300

Morb0 opened this issue Aug 19, 2024 · 1 comment

Comments

@Morb0
Copy link

Morb0 commented Aug 19, 2024

Why embeds on reply edit always replaced and not made as allowed_mentions for example?
Because of that I can't just edit components without not cloning previous embeds.

poise/src/reply/builder.rs

Lines 174 to 178 in 5750259

if let Some(allowed_mentions) = allowed_mentions {
builder = builder.allowed_mentions(allowed_mentions);
}
builder.embeds(embeds)

UPD.
Okay, I found that for CreateReply field embeds is not Option. Why?

@TapGhoul
Copy link

TapGhoul commented Sep 7, 2024

I've found this to be strange too. Though I can see one issue - given existing embeds are kept, there's no way to just clear out all embeds explicitly if this was changed with the current API.

Maybe add

fn embeds(&self, embeds: Vec<serenity::CreateEmbed>) -> Self {
    self.embeds = Some(embeds);
    self
}

that allows the list to be set explicitly? And then replace the existing embed fn with something like

pub fn embed(mut self, embed: serenity::CreateEmbed) -> Self {
    self.embeds.get_or_insert_with(|| Vec::capacity(1)).push(embed);
    self
}

get_or_insert_default() is still nightly only, otherwise I'd suggest that (though the capacity preallocation is a nice to have)

TapGhoul added a commit to TapGhoul/poise that referenced this issue Sep 11, 2024
TapGhoul added a commit to TapGhoul/poise that referenced this issue Sep 11, 2024
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 a pull request may close this issue.

2 participants