Skip to content
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

Remove Sync22 #5014

Merged
merged 3 commits into from
Aug 30, 2024
Merged

Remove Sync22 #5014

merged 3 commits into from
Aug 30, 2024

Conversation

ChrisPenner
Copy link
Contributor

@ChrisPenner ChrisPenner commented May 28, 2024

Overview

Following #5013 Sync22 is no longer used, and also doesn't have any significant testing in place.

I can see arguments for either keeping it or removing it;
In theory it's nice to have some sort of way to sync code between local codebases, even though we don't have any user workflows for doing that right now,

On the other hand, it represents a pretty big chunk of low-level sqlite code that I've often found myself needing tweak or update to keep up-to-date, and I don't believe it's tested well at the moment, esp. once git is removed.

Implementation notes

  • Removes Sync22 functionality

Test coverage

  • This chunk of code isn't used and doesn't seem to be tested.

@ChrisPenner ChrisPenner marked this pull request as ready for review May 28, 2024 17:41
Base automatically changed from cp/remove-git-support to trunk May 29, 2024 12:28
@aryairani
Copy link
Contributor

and also doesn't have any significant testing in place.

Is this just a matter of coming up with some tests? I could imagine: syncing a project branch from one codebase to another; and then some kind of consistency check i guess

it represents a pretty big chunk of low-level sqlite code that I've often found myself needing tweak or update to keep up-to-date

would you say more about these tweaks and updates?

@ceedubs
Copy link
Contributor

ceedubs commented May 29, 2024

I have no idea what Sync22 is, but #3837 came up as something that could potentially benefit from this code.

@ChrisPenner
Copy link
Contributor Author

would you say more about these tweaks and updates?

Mostly it was just when I was splitting up the serializers/deserializers so I could easier use them in Share I had to patch a few conversion functions and stuff in Sync22; maybe now that that's done it'll come up less often 😄 .

I tend to lose trust in codepaths that aren't being exercised often, E.g. MigrateSchema1to2 we KNOW is pretty broken at this point because it's not tested and has been sitting around to bitrot for too long.

@ChrisPenner
Copy link
Contributor Author

Arya and I decided if we do want to support codebase-to-codebase direct sync we should do it using SyncV2 instead; that way we only have a single implementation for both Share and UCM; plus that way you can "sync" to a file to send to your buddy without needing to share your whole codebase.

@ChrisPenner ChrisPenner added the ready-to-merge Apply this to a PR and it will get merged automatically once CI passes and 1 reviewer has approved label Aug 30, 2024
@aryairani aryairani merged commit bbf7352 into trunk Aug 30, 2024
1 check passed
@aryairani aryairani deleted the cp/remove-sync22 branch August 30, 2024 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Apply this to a PR and it will get merged automatically once CI passes and 1 reviewer has approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants