-
Notifications
You must be signed in to change notification settings - Fork 566
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
Rewrite collectors #2240
Merged
Merged
Rewrite collectors #2240
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
40bff1a
to
4fe2754
Compare
This was referenced Oct 26, 2022
Closed
kangalio
added a commit
to kangalio/serenity
that referenced
this pull request
Oct 27, 2022
serenity-rs#2228 changed CreateAutocompleteResponse to be a stand-alone response struct into just the representation of the `data` response struct field. That's because you're supposed to embed it in the `CreateInteractionResponse` enum now. But I forgot that there exists `create_autocomplete_response`, a convenience function for `create_response`, which took CreateAutocompleteResponse directly. After serenity-rs#2228, create_autocomplete_response still compiled, but now didn't send a proper response to discord but just its `data` field. This PR removes create_autocomplete_response as a temporary fix. For now, you should use create_response as for any other interaction response. Later, there should be a PR that re-adds convenience interaction response methods, but do it proper this time, i.e. not just for autocomplete, but also for the other response types. This undertaking should happen after serenity-rs#2229 is merged (which itself is currently waiting on serenity-rs#2240), because otherwise we'd have to duplicate the convenience methods for CommandInteraction, ComponentInteraction, and ModalInteraction.
I added some comments in collector.rs to explain how make_specific_collector works: make_specific_collector!(
// First line has name of the collector type, and the type of the collected items.
ComponentInteractionCollector, ComponentInteraction,
// This defines the extractor pattern, which extracts the data we want to collect from an Event.
Event::InteractionCreate(InteractionCreateEvent {
interaction: Interaction::Component(interaction),
}) => interaction,
// All following lines define built-in filters of the collector.
// Each line consists of:
// - the filter name (the name of the generated builder-like method on the collector type)
// - filter argument type (used as argument of the builder-like method on the collector type)
// - filter expression (this expressoin must return true to let the event through)
author_id: UserId => interaction.user.id == *author_id,
channel_id: ChannelId => interaction.channel_id == *channel_id,
guild_id: GuildId => interaction.guild_id.map_or(true, |x| x == *guild_id),
message_id: MessageId => interaction.message.id == *message_id,
custom_ids: Vec<String> => custom_ids.contains(&interaction.data.custom_id),
); |
88a89c7
to
6ac5d23
Compare
What's blocking this PR? |
arqunis
approved these changes
Nov 9, 2022
arqunis
added a commit
to arqunis/serenity
that referenced
this pull request
Nov 9, 2022
This reverts commit c259280. I presumed it was Github being confused with which changes to show when merging the pull request (serenity-rs#2240), but they were real, reverting a few commits on `next` as a result.
arqunis
pushed a commit
that referenced
this pull request
Nov 13, 2022
arqunis
added a commit
to arqunis/serenity
that referenced
this pull request
Nov 13, 2022
This reverts commit c259280. I presumed it was Github being confused with which changes to show when merging the pull request (serenity-rs#2240), but they were real, reverting a few commits on `next` as a result.
mkrasnitski
pushed a commit
to mkrasnitski/serenity
that referenced
this pull request
Feb 28, 2023
mkrasnitski
pushed a commit
to mkrasnitski/serenity
that referenced
this pull request
Feb 28, 2023
This reverts commit c259280. I presumed it was Github being confused with which changes to show when merging the pull request (serenity-rs#2240), but they were real, reverting a few commits on `next` as a result.
mkrasnitski
pushed a commit
to mkrasnitski/serenity
that referenced
this pull request
May 18, 2023
mkrasnitski
pushed a commit
to mkrasnitski/serenity
that referenced
this pull request
May 18, 2023
This reverts commit c259280. I presumed it was Github being confused with which changes to show when merging the pull request (serenity-rs#2240), but they were real, reverting a few commits on `next` as a result.
mkrasnitski
pushed a commit
to mkrasnitski/serenity
that referenced
this pull request
May 30, 2023
mkrasnitski
pushed a commit
to mkrasnitski/serenity
that referenced
this pull request
May 30, 2023
This reverts commit c259280. I presumed it was Github being confused with which changes to show when merging the pull request (serenity-rs#2240), but they were real, reverting a few commits on `next` as a result.
mkrasnitski
pushed a commit
to mkrasnitski/serenity
that referenced
this pull request
Sep 21, 2023
mkrasnitski
pushed a commit
to mkrasnitski/serenity
that referenced
this pull request
Sep 21, 2023
This reverts commit c259280. I presumed it was Github being confused with which changes to show when merging the pull request (serenity-rs#2240), but they were real, reverting a few commits on `next` as a result.
mkrasnitski
pushed a commit
to mkrasnitski/serenity
that referenced
this pull request
Oct 17, 2023
mkrasnitski
pushed a commit
to mkrasnitski/serenity
that referenced
this pull request
Oct 17, 2023
This reverts commit c259280. I presumed it was Github being confused with which changes to show when merging the pull request (serenity-rs#2240), but they were real, reverting a few commits on `next` as a result.
mkrasnitski
pushed a commit
to mkrasnitski/serenity
that referenced
this pull request
Oct 24, 2023
mkrasnitski
pushed a commit
to mkrasnitski/serenity
that referenced
this pull request
Oct 24, 2023
This reverts commit c259280. I presumed it was Github being confused with which changes to show when merging the pull request (serenity-rs#2240), but they were real, reverting a few commits on `next` as a result.
arqunis
pushed a commit
to arqunis/serenity
that referenced
this pull request
Oct 24, 2023
arqunis
added a commit
to arqunis/serenity
that referenced
this pull request
Oct 24, 2023
This reverts commit c259280. I presumed it was Github being confused with which changes to show when merging the pull request (serenity-rs#2240), but they were real, reverting a few commits on `next` as a result.
arqunis
pushed a commit
to arqunis/serenity
that referenced
this pull request
Oct 24, 2023
arqunis
added a commit
to arqunis/serenity
that referenced
this pull request
Oct 24, 2023
This reverts commit c259280. I presumed it was Github being confused with which changes to show when merging the pull request (serenity-rs#2240), but they were real, reverting a few commits on `next` as a result.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
breaking change
The public API is changed, resulting in miscompilations or unexpected new behaviour for users
builder
Related to the `builder` module.
cache
Related to the `cache`-feature.
client
Related to the `client` module.
collector
Related to the `collector` module.
enhancement
An improvement to Serenity.
examples
Related to Serenity's examples.
gateway
Related to the `gateway` module.
http
Related to the `http` module.
model
Related to the `model` module.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
They were overengineered and a bit complicated to use. Now, each collector has just one corresponding type, with methods collect_stream() or collect_single(), depending on what you need.
The implementation is much simplified, too.
Previously, each collector type had a method on Context, a method on ShardMessenger, a variant on ShardRunnerMessage, a branch in ShardRunner::handle_filters and a branch in ShardRunner::handle_rx_value. But they all fundamentally do the same thing; so I replaced all of that with a single CollectorCallback system that all collectors use. This reduces the collector-related code in the gateway to like a couple lines of code.
collector.rs is now a single file, with a collector primitive function called
collect()
which is enough to implement any possible collector. The existing predefined collectors for messages, component interactions, modal interactions, reactions, and generic events still exist though. Those collector structs are generated using a macro, because their structure is very consistent (due to them being mostly dumb wrapper around ´collect()`)I removed the
removed
filter on ReactionCollector because listening for removed reactions seems very niche, and (like anything else), can be achieved with thecollect()
primitive as well. So ReactionCollector now always collects added reactions. Thecollect()
docs show an example of how to collect removed reactions.I also removed the filters on EventCollector. Filtering for specific events is better done using
collect()
with a match statement over Event, like is shown incollect()
's example