-
-
Notifications
You must be signed in to change notification settings - Fork 132
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
base: next
Are you sure you want to change the base?
Conversation
fb1cd56
to
cf6616d
Compare
There was a problem hiding this 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
Custom { | ||
/// Emoji identifier. | ||
id: Id<EmojiMarker>, |
There was a problem hiding this comment.
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.
Please resolve the merge conflicts and address zeylas review. |
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. |
Whilst working on the HTTP ratelimiter, I discovered this specialized
ReactionType
model. Like other specialized models, they should go intotwilight_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.