-
Notifications
You must be signed in to change notification settings - Fork 329
feat(sdk): Add creation of indexes and indexing of messages #5505
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
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5505 +/- ##
==========================================
+ Coverage 88.57% 88.58% +0.01%
==========================================
Files 339 339
Lines 93627 93649 +22
Branches 93627 93649 +22
==========================================
+ Hits 82927 82957 +30
+ Misses 6565 6558 -7
+ Partials 4135 4134 -1 ☔ View full report in Codecov by Sentry. |
CodSpeed Performance ReportMerging #5505 will not alter performanceComparing Summary
|
d771525
to
815f511
Compare
Handing to @poljar as I think you are looking after this. If this is just a standard review, feel free to hand it back to me. |
8c7da74
to
7d21856
Compare
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.
Sweet, excited to see this integration happen 🥳
I've got a few suggestions about the API shape and the integration into the event cache, but you're off a great start!
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.
A few more questions, but this is almost good to land 🥳
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, approving with some review comments, we can refine it later 👍 Good job!
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.
See new responses in review 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.
I hear that some changes are missing from this PR 👀
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.
Alright, let's ship it, and we'll work on improvements in subsequent PRs. Good job!
pub(crate) async fn index_event( | ||
&self, | ||
event: AnySyncMessageLikeEvent, | ||
) -> Result<(), IndexError> { | ||
self.client.search_index().lock().await.handle_event(event, self.room_id()) | ||
} |
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 do realize only now; it might be better to have the callers of this method directly call the Client
's method, since if they have a Room
around they have a Client
around. And having Room
-methods is mostly nice for public APIs (like search
below), otherwise there's no need to have an indirect call structure like this.
862d604
to
7267848
Compare
Integrate matrix-sdk-search into matrix-sdk.
When a room is joined, a corresponding index is created.
When a message is received via sync or via a back-pagination, it is added to the corresponding room's index.
Signed-off-by: Shrey Patel [email protected]