-
Notifications
You must be signed in to change notification settings - Fork 254
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
Filter one or multiple events by type from an EventSubscription #461
Conversation
I decided to wrap all filtered events in a |
/// Return only specific events matching the tuple of 1 or more event | ||
/// types that has been provided as the `Filter` type parameter. | ||
pub fn filter_events<Filter: EventFilter>(self) -> FilterEvents<'a, Self, T, Filter> { | ||
FilterEvents::new(self) | ||
} |
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.
Added.
// You should have received a copy of the GNU General Public License | ||
// along with subxt. If not, see <http://www.gnu.org/licenses/>. | ||
|
||
//! Filtering individual events from subscriptions. |
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.
This file is new.
#[cfg(test)] | ||
mod tests { | ||
pub(crate) mod test_utils { |
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 moved some common event test bits to this module so that I could use them in filter_events tests, too.
@@ -370,7 +274,7 @@ impl<'a, T: Config, Evs: Decode> Events<'a, T, Evs> { | |||
/// | |||
/// **Note:** This method internally uses [`Events::iter_raw()`], so it is safe to | |||
/// use even if you do not statically know about all of the possible events. | |||
pub fn find_first_event<Ev: Event>(&self) -> Result<Option<Ev>, BasicError> { | |||
pub fn find_first<Ev: Event>(&self) -> Result<Option<Ev>, BasicError> { |
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.
renamed to be consistent with removal of "event" prefix from similar functions previously.
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.
It threw me off at first when reading the examples, but in the end I think it's the right call.
/// | ||
/// Unlike [`Events::iter_raw()`] this consumes `self`, which can be useful | ||
/// if you need to store the iterator somewhere and avoid lifetime issues. | ||
pub fn into_iter_raw( |
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.
Added to support filter_events
; we need to consume self
so that we can work with it in our FilterEvents Stream impl.
Since I was very unhelpful in combining the addition of the |
let mut balance_events = api.events().subscribe().await?.filter_events::<( | ||
polkadot::balances::events::Withdraw, | ||
polkadot::balances::events::Transfer, | ||
polkadot::balances::events::Deposit, | ||
)>(); |
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.
Added this new example to show off the "filter multiple events" case.
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.
Nice.
@@ -370,7 +274,7 @@ impl<'a, T: Config, Evs: Decode> Events<'a, T, Evs> { | |||
/// | |||
/// **Note:** This method internally uses [`Events::iter_raw()`], so it is safe to | |||
/// use even if you do not statically know about all of the possible events. | |||
pub fn find_first_event<Ev: Event>(&self) -> Result<Option<Ev>, BasicError> { | |||
pub fn find_first<Ev: Event>(&self) -> Result<Option<Ev>, BasicError> { |
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.
It threw me off at first when reading the examples, but in the end I think it's the right call.
events: Option< | ||
Box< | ||
dyn Iterator< | ||
Item = Result< | ||
FilteredEventDetails<T::Hash, Filter::ReturnType>, | ||
BasicError, | ||
>, | ||
> + 'a, | ||
>, | ||
>, |
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.
This made me giggle, you can really tell cargo fmt
was cussing and sweating here.
/// `Event` types, it will return a corresponding tuple of `Option`s, where | ||
/// exactly one of these will be `Some(event)` each iteration. |
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.
Just curious, do you know of other APIs that adopt this approach to filtering? It's a tricky thing to get right (i.e. be ergonomic).
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 don't! I suppose an alternative would be to generate/use an enum with the right number of variants for each case, which, come to think of it, might not be such a bad idea..
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.
(although it would be less teachable and intuitive perhaps, so I think the current approach will do for now!)
|
||
/// This trait is implemented for tuples of Event types; any such tuple (up to size 8) can be | ||
/// used to filter an event subscription to return only the specified events. | ||
pub trait EventFilter: private::Sealed { |
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.
Maybe move this type higher up in the file, given it's used extensively by the rest of the code?
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 had a look, and FilterEvents
/FilteredEventDetails
both feel like they deserve to be higher to me, since they are the "api" we expose here, and EventFilter
is just a part of how it works (albeit an important one!).
Co-authored-by: David <dvdplm@gmail.com>
In this PR, I split
events.rs
into several files (it was getting rather large) and add afiltered_events
method/file, which allows you to extract one or more events from an event subscription stream.See the examples and unit tests for usage!
Closes #457