Initial implementation of an in-memory version of the API #10
Conversation
|
This will resolve #3 |
There was a problem hiding this comment.
LGTM, with some comments. @fluffyemily, would you mind reviewing, too? You have a lot more Rust experience, and I think you'd have more insightful feedback here.
| pub event: String, | ||
| // TODO: Think about more complex types -- Or do we want callers just | ||
| // storing JSON here? Can we use std::any::Any and hope to serialize it? | ||
| pub data: String |
There was a problem hiding this comment.
Maybe we can define our own trait for the data, and implement it for strings, numbers, timestamps, and so on? Though, I don't think that will work if we want to call the API from Swift or C++...without more type info, we wouldn't know how to interpret the *c_void that the FFI gives us. Leaving it as a string is fine for now.
There was a problem hiding this comment.
Yeah, that was my thought too. And since Any exists already, it seems like the trait we might want to use if we were going to go the route of allowing anything.
| } | ||
|
|
||
| impl PlaceAction { | ||
| pub fn new(url: Url, event: &str, data: &str) -> PlaceAction { |
There was a problem hiding this comment.
We might need to jump through some hoops if we want to expose Url via FFI, but we can work out how to do that later. Just noting it for now.
There was a problem hiding this comment.
Yeah, I figured String would honestly not necessarially be any better, given that it's still a complex type (although, I guess it's more likely to be already solved for String than for Url).
There was a problem hiding this comment.
@fluffyemily's been looking at how to expose complex types over the FFI.
|
|
||
| impl Store for MemoryStore { | ||
| fn record_place_action(&mut self, action: PlaceAction) { | ||
| if let Some(v) = self.data.get_mut(&action.url) { |
There was a problem hiding this comment.
I think HashMap::entry exists for cases just like this! 🎉
There was a problem hiding this comment.
Noted, I avoided it due to the extra clone it requires (rust-lang/rfcs#1769), but I'm probably overthinking it and that does not matter for Urls.
| self.data.insert(action.url.clone(), vec![action]); | ||
| } | ||
|
|
||
| fn query_place_actions<'a>(&'a self, url: &Url) -> &'a [PlaceAction] { |
There was a problem hiding this comment.
I wonder if we should return an owned copy of the Vec, instead of tying it to the store's lifetime. On the one hand, that's inefficient; on the other, I wonder how we're going to enforce this in the C++/Swift wrapper. Maybe we can have another layer do the cloning...or maybe this will all work fine as is!
There was a problem hiding this comment.
My concern is that it could be an enormous number of records, since we do no filtering. Consider all the actions users might have taken on websites like facebook or reddit.
Given that, I think even returning a slice is bad here. We probably should be returning an iterator of some sort (or a cursor)? But I don't know how we'd do that with FFI either.
There was a problem hiding this comment.
Honestly, I wouldn't worry about slices and iterators and cloning and ownership at this stage.
If you're just exploring concepts, you don't need efficiency. If you're not just exploring API modeling concepts, and you're worried about scale, then you don't want a string-based key-value store anyway.
I'd be surprised if a consumer ever had a good reason to call query_place_actions.
You might be interested in how SQLite on Android uses cursor windows.
ghost
requested a review
from 
This isn't a great API, but it's probably enough to get started with.
Kit, do you mind reviewing?