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

Added additional reaction controls to channel_id and message structs #2533

Merged
merged 8 commits into from
Sep 9, 2023

Conversation

Collin-Swish
Copy link
Contributor

#2532 In reference to this issue.

@github-actions github-actions bot added the model Related to the `model` module. label Sep 6, 2023
Copy link
Collaborator

@mkrasnitski mkrasnitski left a comment

Choose a reason for hiding this comment

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

Could you add a GuildChannel::delete_reactions wrapper method as well? (A PrivateChannel::delete_reactions method doesn't make sense, because only reactions by the current user can be removed in a private channel.)

Also, make sure to run cargo +nightly fmt.

Comment on lines 282 to 286
/// Deletes all of the [`Reaction`]s associated with the provided message id.
///
/// **Note**: Requires the [Manage Messages] permission.
///
/// [Manage Messages]: Permissions::MANAGE_MESSAGES
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should include an # Errors section.

src/model/channel/message.rs Outdated Show resolved Hide resolved
Comment on lines 285 to 294
cache_http
.http()
.as_ref()
.delete_reaction(
self.channel_id.0,
self.id.0,
user_id.map(|uid| uid.0),
&reaction_type.into(),
)
.await
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be reworked to just a wrapper around ChannelId::delete_reaction. The same goes for Message::delete_reactions, now that ChannelId::delete_reactions exists.

src/model/channel/message.rs Outdated Show resolved Hide resolved
Comment on lines 274 to 275
/// Returns [`Error::Http`] if the currend user did not perform the reaction,
/// and lacks permission
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Returns [`Error::Http`] if the currend user did not perform the reaction,
/// and lacks permission
/// Returns [`Error::Http`] if the current user did not perform the reaction, or lacks
/// permission.

formatting

@arqunis arqunis added the enhancement An improvement to Serenity. label Sep 7, 2023
Copy link
Collaborator

@mkrasnitski mkrasnitski left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Could you also change the implementation of Message::delete_reactions to be:

self.channel_id.delete_reactions(cache_http.http(), self.id).await

src/model/channel/guild_channel.rs Outdated Show resolved Hide resolved
src/model/channel/message.rs Outdated Show resolved Hide resolved
Collin-Swish and others added 3 commits September 9, 2023 11:38
Co-authored-by: Michael Krasnitski <42564254+mkrasnitski@users.noreply.github.com>
Co-authored-by: Michael Krasnitski <42564254+mkrasnitski@users.noreply.github.com>
@arqunis arqunis merged commit 1503d53 into serenity-rs:current Sep 9, 2023
1 check passed
@mkrasnitski
Copy link
Collaborator

This slipped past in review, but Message::delete_reaction should take &self instead of self as it doesn't make sense to take ownership. Conversely, ChannelId::delete_reactions should in fact take self instead of &self, because ChannelId is Copy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement to Serenity. model Related to the `model` module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants