-
Notifications
You must be signed in to change notification settings - Fork 171
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
Fix: adds new filters functions #1281
base: main
Are you sure you want to change the base?
Conversation
#1280 Since is my very first commit I wasn't sure about that. |
Yes, this is correct. We were discussing having separate methods only because of compatibility with server version. Instead we landed on this:
|
ok, I changed the code. I hope the way I instantiate the config is fine. |
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.
Hey!
Thank you for your contribution!
This looks pretty good.
Few comments made.
async-nats/tests/kv_tests.rs
Outdated
@@ -795,6 +796,28 @@ mod kv { | |||
keys.sort(); | |||
assert_eq!(vec!["bar", "foo"], keys); | |||
|
|||
|
|||
let mut keys_with_filter = kv |
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 would add few more keys, and try a bit more complex filters and check them.
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 added more tests following the docs here
I think filters in the format foo.b*
are not supported, as *
can only replace whole words, am I right?
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.
Functionally looks good, but one improvement requested.
@TheGhoul21 are you interested in finishing this PR? 🙂 |
152cb37
to
24e985a
Compare
I pushed the changes you suggested. |
@TheGhoul21 You need to put the function behind the feature flag #[cfg(feature = "server_2_10")] |
match (filters.next(), filters.next()) {
(Some(first), None) => {
config.filter_subject = first;
}
(Some(first), Some(second)) => {
#[cfg(feature = "server_2_10")]
{
config.filter_subjects = vec![first, second];
config.filter_subjects.extend(filters);
}
#[cfg(not(feature = "server_2_10"))]
{
config.filter_subject = first;
// maybe a warning
}
}
_ => {}
} I hate the duplicate code though |
Ah, right. @TheGhoul21 The sole reason for utilising the feature flag is to avoid such situations where not all passed filters are properly set. I'm afraid that duplication is the way to go here. |
#[cfg(not(feature = "server_2_10"))] | ||
{ | ||
config.filter_subject = first; | ||
// maybe a warning |
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 is not the way to go.
As said before - copy-paste is the way to go, for now.
hey is this gona be finally merged? looks like its failing on a macos thing, maybe the contributor doesnt have a mac? not familiar with your build scripts and i cant see the log of the run history. |
The PR is not finished. If it will not be picked up soon, I will finish it instead. EDIT: The mac failure is a flaking test. The github actions mac machines are quite overutilized. |
No description provided.