Add convenience get_most_recent method to fetch newest history metadata with a limit
#7002
Conversation
issammani
changed the title
get_most_recent method to fetch newest history metadata with a limit
mhammond
left a comment
mhammond
left a comment
There was a problem hiding this comment.
This looks great! I think you will need an addition to the UDL too? And I'm wondering if we should we maybe just replace get_since() - it sounds like you are saying it's not useful so it seems likely Android might want the same thing? We wont block on Android changing, but it might be nice to leave a breadcrumb here for future us.
(It's unfortunate this "history-metadata" stuff still exists - it's basically "normal" history with some other "view time" stuff that's not synced - but it is what is it is :)
|
|
||
| note_observation!(&conn, | ||
| url "https://example.com/1", | ||
| view_time Some(10), |
There was a problem hiding this comment.
we should probably have this identical for all visits to make it clear it doesn't matter. Related, this might be racey in that it probably assumes incrementing timestamps between observations, which might not be true, so the order back is different. I'm not immediately sure what to do about that :( Maybe ignore it if other tests rely on it, sqlite doing a full transaction before the timestamp advances seems unlikely to be a problem for a while?
There was a problem hiding this comment.
Good callout. I added both with same observation and different ones in 77cca7f. Let me know if you think we need to test more variations.
There was a problem hiding this comment.
Related, this might be racey in that it probably assumes incrementing timestamps between observations,
Yeah I can see how that can be a problem. I don't see anything in the other tests that attempts to solve it other than maybe some thread::sleep that might have been added for this ?
There was a problem hiding this comment.
yeah, maybe a comment and a super-short sleep between reecords will do for now
issammani
force-pushed
the
feat/add-convenience-method-history-meta-data
branch
from
60b2080 to
50eee6b
Compare
True. I got so used to the macros now hahah. Added in 50eee6b
Yeah that's the plan. I will also change Android after this lands and we can remove it after. For some reason, I always end up breaking android when I do breaking changes 🙈 So this seems safer. Added ticket and comment on top of method to track this in d7eaab9 😄 |
|
|
||
| ### Swift | ||
| - Added `@unchecked Sendable` to classes that conform to `FeatureManifestInterface`. ([#6963](https://github.com/mozilla/application-services/pull/6963) | ||
| - Added `@unchecked Sendable` to classes that conform to `FeatureManifestInterface`. ([#6963](https://github.com/mozilla/application-services/pull/6963)) |
There was a problem hiding this comment.
Was missing a ) at the end and it bothered me 😂
|
|
||
| note_observation!(&conn, | ||
| url "https://example.com/1", | ||
| view_time Some(10), |
There was a problem hiding this comment.
yeah, maybe a comment and a super-short sleep between reecords will do for now
mergify
bot
dismissed
mhammond’s stale review
The pull request has been modified, dismissing previous reviews.
2 participants
Pull Request checklist
This patch adds a new convenience method
get_most_recentto retrieve the newest history metadata entries ordered by updated_at DESC with a configurable limit. Without this method, we have to useget_since(start: min_i64)and pop first n elements that we need from the vec which can be wasteful at times. Usingget_most_recentshould give back exactly the number of items we need ordered correctly.[ci full]to the PR title.