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

refactor!: move RequestReactionType to model #2162

Draft
wants to merge 4 commits into
base: next
Choose a base branch
from

Conversation

vilgotf
Copy link
Member

@vilgotf vilgotf commented Feb 26, 2023

Whilst working on the HTTP ratelimiter, I discovered this specialized ReactionType model. Like other specialized models, they should go into twilight_model::http. Additionally, the custom emoji optional name field is removed as provides no utility.

I also temporarily depended on this change, but I've since refactored that code.

@github-actions github-actions bot added c-http Affects the http crate c-model Affects the model crate m-breaking change Breaks the public API. t-refactor Refactors APIs or code. labels Feb 26, 2023
@vilgotf vilgotf force-pushed the vilgotf/model/refactor/http-reactiontype branch from fb1cd56 to cf6616d Compare February 26, 2023 08:17
Copy link
Member

@Erk- Erk- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I think the documentation update is good as well

Comment on lines +13 to +15
Custom {
/// Emoji identifier.
id: Id<EmojiMarker>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel as though we should still have the name here, even if Discord doesn't end up using it. Who knows, there may come a day it does. The documentation for creating a reaction on a message is:

To use custom emoji, you must encode it in the format name:id with the emoji name and emoji id.

This reads to me like the API prefers the name. We should at least give users the ability to provide it so we match the API, regardless of whether it uses it.

@itohatweb
Copy link
Member

Please resolve the merge conflicts and address zeylas review.

@vilgotf vilgotf marked this pull request as draft June 22, 2023 13:14
@vilgotf
Copy link
Member Author

vilgotf commented Jun 22, 2023

This might not be necessary and either way it's on the backburner for now.

@Gelbpunkt
Copy link
Member

This might not be necessary and either way it's on the backburner for now.

Why would this not be necessary? Seems like a reasonable change to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-http Affects the http crate c-model Affects the model crate m-breaking change Breaks the public API. t-refactor Refactors APIs or code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants