Add iterator and accessor to EventManager #69
Conversation
Implement iterator and accessor for EventManager, so we could iterate over all subscribers regisgtered. Also improve test cases in basic_event_manager.rs. Signed-off-by: Liu Jiang <[email protected]>
Add two unit test cases for SubscriberId and remote endpoint. Signed-off-by: Liu Jiang <[email protected]>
| @@ -168,6 +168,9 @@ pub trait SubscriberOps { | |||
| /// Removes the subscriber corresponding to `subscriber_id` from the watch list. | |||
| fn remove_subscriber(&mut self, subscriber_id: SubscriberId) -> Result<Self::Subscriber>; | |||
|
|
|||
| /// Returns a reference to the subscriber corresponding to `subscriber_id`. | |||
| fn subscriber_ref(&self, subscriber_id: SubscriberId) -> Option<&Self::Subscriber>; | |||
There was a problem hiding this comment.
Can we return a Result here as well so that it in line with the other getter we have for subscribers (i.e. the subscriber_mut)?
There was a problem hiding this comment.
Can you also add a test for when the subscriber was removed, and then we call subscriber_ref?
| @@ -35,6 +35,15 @@ impl App { | |||
| let _ = self.event_manager.run_with_timeout(100); | |||
| } | |||
|
|
|||
| fn iter(&mut self) { | |||
There was a problem hiding this comment.
This check doesn't really fit here. This is the example that we're also referencing in the public documentation, so I'd like to keep it as close as we can to a real use life use case.
How about we move this to a separate test instead of having it part of the App interface? We can add a test_iter and move the following asserts there after initializing the subscribers.
| use super::*; | ||
|
|
||
| #[test] | ||
| fn test_subscriber_id_derives() { |
There was a problem hiding this comment.
Is there any reason for checking the derives? We considered this to be coverage tests, and we avoided them in the past. I am wondering if there is something particular about the SubscriberId that we want to validate, or we could remove them.
| } | ||
|
|
||
| /// An iterator visiting all subscribers in arbitrary order, with immutable references. | ||
| pub(crate) fn iter(&mut self) -> Iter<'_, SubscriberId, T> { |
There was a problem hiding this comment.
Can this use an immutable reference &self? (duplicate of 49ba509#r1168309051)
| @@ -181,6 +187,16 @@ impl<S: MutEventSubscriber> EventManager<S> { | |||
| #[cfg(feature = "remote_endpoint")] | |||
| self.dispatch_endpoint_event(endpoint_event); | |||
| } | |||
|
|
|||
| /// An iterator visiting all subscribers in arbitrary order, with immutable references. | |||
| pub fn iter(&mut self) -> Iter<'_, SubscriberId, S> { | |||
There was a problem hiding this comment.
Can this use an immutable reference &self? (duplicate of 49ba509#r1168308920)
| // Once the app does not need events anymore, the cleanup needs to be called. | ||
| // This is particularly important when the app continues the execution, but event monitoring | ||
| // is not necessary. | ||
| app.cleanup(); | ||
|
|
||
| assert!(app.event_manager.subscriber_ref(id).is_none()); | ||
| assert!(app.event_manager.subscriber_mut(id).is_err()); |
There was a problem hiding this comment.
Can this check the error more explicitly?
E.g. assert_eq!(app.event_manager.subscriber_mut(id), Err(..));
Add iterator and accessor to EventManager, so we could walk all registered subscribers.