Skip to content

refactor(common): LinkedChunk can start by a gap #4653

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

Merged

Conversation

Hywan
Copy link
Member

@Hywan Hywan commented Feb 11, 2025

This patch removes the invariant stating that a LinkedChunk must start by a chunk of type items. This has never been really useful but it's now annoying to have this (with iterative loading of a LinkedChunk via the EventCache, it's now possible to get a gap as the first chunk). Let's remove this invariant.


@Hywan Hywan marked this pull request as ready for review February 11, 2025 14:03
@Hywan Hywan requested a review from a team as a code owner February 11, 2025 14:03
@Hywan Hywan requested review from stefanceriu and bnjbvr and removed request for a team February 11, 2025 14:03
Copy link

codecov bot commented Feb 11, 2025

Codecov Report

Attention: Patch coverage is 93.33333% with 3 lines in your changes missing coverage. Please review.

Project coverage is 85.69%. Comparing base (69588d5) to head (74f927b).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
crates/matrix-sdk-common/src/linked_chunk/mod.rs 93.33% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4653      +/-   ##
==========================================
+ Coverage   85.68%   85.69%   +0.01%     
==========================================
  Files         292      292              
  Lines       33570    33600      +30     
==========================================
+ Hits        28763    28794      +31     
+ Misses       4807     4806       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Glad to see this invariant go 🥳
I'm mostly curious to see the final form of the code, but my comments are all mostly cosmetic and the code LGTM so approving to not block merging of dependent PRs.

This patch removes the invariant stating that a `LinkedChunk` must start
by a chunk of type items. This has never been really useful but it's now
annoying to have this (with iterative loading of a `LinkedChunk` via the
`EventCache`, it's now possible to get a gap as the first chunk). Let's
remove this invariant.
This patch splits the `test_replace_at` test into 3 smaller tests.
This patch updates the `test_replace_item` test to ensure
`Update::ReplaceItem` is correct.
This patch splits the `test_insert_items_at` test into 5 tests.
@Hywan Hywan force-pushed the feat-common-linked-chunk-can-start-by-a-gap branch from e29958f to 74f927b Compare February 11, 2025 15:56
@Hywan
Copy link
Member Author

Hywan commented Feb 11, 2025

I've improved the comments a bit. I've also split the test_insert_items_at tests into 5 tests, and test_replace_at into 3 tests.

@Hywan Hywan merged commit fce7999 into matrix-org:main Feb 11, 2025
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants