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 <gerry@linux.alibaba.com>
Add two unit test cases for SubscriberId and remote endpoint. Signed-off-by: Liu Jiang <gerry@linux.alibaba.com>
andreeaflorescu
left a comment
andreeaflorescu
left a comment
There was a problem hiding this comment.
Sorry for the late review.
| 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?
| 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)
| } | ||
|
|
||
| /// 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)
| 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(..));
3 participants
Add iterator and accessor to EventManager, so we could walk all registered subscribers.