refactor: Remove indirection from crdt packages #3192
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It only ever references itself
Was causing so much indirection and seriously inhibited the readability of the code by losing the known type in the Merge function. Now we can at least go from merkle.lwwr.Merge to core.lwwr.Merge without chasing down all implementations of the interface and wondering if they can all form part of a merkle.lwwr...
There is only one implementation, and by intefacing it out with the same name, it makes tracking down the data flow much harder than it needs to be
The funcs held on it only ever used the 'store' prop and ignored the rest. Not all the types inheriting from it used all of the props on it (e.g. composite was forced to pass an empty string to the mandatory arg it ignores in the constructor). Inheritance is not required to remove code duplication, and it hides important properties from people reading the inheriting types. It also makes the code appear more complicated than it is, especially when introducing new types, as it *looks* like it contains important business logic that must be consistent, but it does not (private funcs do not enforce consistency).
It is never actually used, and instead makes implementations of the MerkleCRDT have this weird self referencing hack via the clock, as well as doubling the implmentations of the already complicated core.ReplicatedData making it appear to be more complex than it actually is.
AndrewSisley
added
area/crdt
AndrewSisley
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3192 +/- ##
===========================================
- Coverage 77.44% 77.43% -0.01%
===========================================
Files 357 357
Lines 34809 34801 -8
===========================================
- Hits 26957 26948 -9
+ Misses 6237 6236 -1
- Partials 1615 1617 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 11 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
fredcarle
approved these changes
Oct 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Relevant issue(s)
Resolves #3191
Description
Removes various items of indirection from the merkle/crdt and core/crdt packages that was making the code quite a lot harder to follow than it need to be.
Done as part of #3038 but broken out for quicker merge and a more focused review (of both).
I suggest reviewing commit by commit, it should be easier to follow the changes, and there are explanations in the commit bodies.