-
Notifications
You must be signed in to change notification settings - Fork 14
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
Filtering supported #225
Filtering supported #225
Conversation
Thank you for the PR |
cc @wolf4ood |
src/consumer.rs
Outdated
impl FilterConfiguration { | ||
pub fn new( | ||
filter_values: Vec<String>, | ||
predicate: impl Fn(&Message) -> bool + 'static + Send + Sync, |
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.
Even if normally it is used to avoid "false positives" I think that predicate is Optional and the user can skip it if not necessary @Gsantomaggio to confirm
tests/integration/consumer_test.rs
Outdated
String::from_utf8(message.data().unwrap().to_vec()).unwrap_or("".to_string()) | ||
== "filtering".to_string() | ||
}, | ||
false, |
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.
If you can also add a test for match_unfiltered set to "true"
Thank you very much for your contribution @JiaYingZhang It looks good to me. If you can also add an example inside the example folder. Something similar on what you already did in the integration test for producer and consumer |
@DanielePalaia I will add the remaining parts as you suggested. |
@DanielePalaia update for your suggest
thank you |
0996915
to
87f09c9
Compare
For me it is good to merge. There maybe a few improvements on the example to do but we can do it when we will create the tutorials for this functionality. The action is failing for a rate limit issue on codecov but tests are passing. @wolf4ood for final approval/comments |
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.
Thanks a lot
Filtering supported