journal: add support for match filters #17
Conversation
|
|
||
| /// Adds a match by which to filter the entries of the journal. | ||
| /// If a match is applied, only entries with this field set will be iterated. | ||
| pub fn match_add<T: Into<Vec<u8>>>(&self, key: &str, val: T) -> Result<&Journal> { |
There was a problem hiding this comment.
I could use an explanation on why &mut self isn't needed here: it appears that these filtering functions are mutating internal state of the journal object.
As we're an ffi interface, it's true that we don't need to exactly follow the & and &mut meanings as provided in rust (I believe C's abi allows us a bit more leeway), but it seems like it'd be useful in this case to make it clear that the journal's internal state is being modified.
There was a problem hiding this comment.
I understand the concern. This is somehow similar to other rust types providing interior mutability, minus the explicit borrowing part. And this is something that doesn't concern only this new match subsystem: seek and next_record also alters internal cursor position.
I think that conveying this semantic via comment is neither efficient nor idiomatic. What about moving to explicit &mut self, even if not strictly required by rust, to let the caller be aware of this?
There was a problem hiding this comment.
That seems fine to me (and we probably should at some point adjust the existing methods to do the same).
lucab
force-pushed
the
to-upstream/journal-matches
branch
2 times, most recently
from
40de04d to
66f3e4e
Compare
|
@jmesmon as discussed, I've moved the new methods to take |
|
On the returns for most of these functions, it looks like a |
This commit introduces support for filtering journal entries. To filter results while iterating a Journal, it is now possible to add filtering terms, AND/OR-ing them in predicates as well as resetting back to an unfiltered iterator.
This commit changes some methods signature to take mutable references wherever the FFI implies some internal status mutation by the C API. While this is not stricly needed by Rust rules, it helps in explicitly conveying the semantic across the layers.
lucab
force-pushed
the
to-upstream/journal-matches
branch
from
66f3e4e to
84cc2ee
Compare
|
You are right. The rationale behind the arbitrary return type was to make chaining easier (once safe navigation RFC is implemented), but I forgot to change it when updating. I amended it now, and also added a (superfluous) flush+match_add chain in the test just to cover that part. PTAL. |
|
Looks good, merged. Thanks. |
This commit introduces support for filtering journal entries.
To filter results while iterating a Journal, it is now possible
to add filtering terms, AND/OR-ing them in predicates as well as
resetting back to an unfiltered iterator.