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

[RNET-992] Add sync support for collections in mixed #3556

Merged
merged 21 commits into from
Apr 9, 2024

Conversation

rorbech
Copy link
Contributor

@rorbech rorbech commented Mar 13, 2024

Description

This adds sync support for collections in mixed fields.

Fixes RNET-992

TODO

  • Changelog entry Collections in mixed is already part of the changelog.
  • Tests

Copy link

coveralls-official bot commented Mar 14, 2024

Pull Request Test Coverage Report for Build 8611831031

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 81.028%

Totals Coverage Status
Change from base Build 8598129525: 0.0%
Covered Lines: 6791
Relevant Lines: 8233

💛 - Coveralls

@rorbech rorbech changed the title Add sync support for collections in mixed [RNET-992] Add sync support for collections in mixed Mar 15, 2024
@rorbech rorbech added the no-changelog Used to skip the changelog check label Mar 15, 2024
@rorbech rorbech force-pushed the cr/collections-in-mixed-sync branch from 6a05037 to 9bbfcb4 Compare March 18, 2024 09:55
@rorbech rorbech marked this pull request as ready for review March 20, 2024 15:15
@rorbech rorbech requested review from papafe and nirinchev and removed request for papafe March 20, 2024 15:16
.github/workflows/pr.yml Outdated Show resolved Hide resolved
Tools/DeployApps/BaasClient.cs Outdated Show resolved Hide resolved
@rorbech rorbech requested a review from nirinchev April 3, 2024 09:34
@rorbech rorbech requested a review from papafe April 3, 2024 09:34
Copy link
Contributor

@papafe papafe left a comment

Choose a reason for hiding this comment

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

Overall it looks good! Just some comments

Tests/Realm.Tests/Sync/DataTypeSynchronizationTests.cs Outdated Show resolved Hide resolved
Tests/Realm.Tests/Sync/DataTypeSynchronizationTests.cs Outdated Show resolved Hide resolved
Tests/Realm.Tests/Sync/DataTypeSynchronizationTests.cs Outdated Show resolved Hide resolved
Tests/Realm.Tests/Sync/DataTypeSynchronizationTests.cs Outdated Show resolved Hide resolved
Tests/Realm.Tests/Sync/DataTypeSynchronizationTests.cs Outdated Show resolved Hide resolved
Tests/Realm.Tests/Sync/DataTypeSynchronizationTests.cs Outdated Show resolved Hide resolved
Tests/Realm.Tests/Sync/DataTypeSynchronizationTests.cs Outdated Show resolved Hide resolved
Tests/Realm.Tests/Sync/DataTypeSynchronizationTests.cs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@@ -236,7 +246,7 @@ public class DataTypeSynchronizationTests : SyncTestBase
public void Dict_Binary() => TestDictionaryCore(o => o.ByteArrayDict, TestHelpers.GetBytes(10), TestHelpers.GetBytes(15), (a, b) => a.SequenceEqual(b));

[Test]
public void Property_Binary() => TestPropertyCore(o => o.ByteArrayProperty, (o, rv) => o.ByteArrayProperty = rv, TestHelpers.GetBytes(5), TestHelpers.GetBytes(10), (a, b) => a!.SequenceEqual(b!));
public void Property_Binary() => TestPropertyCore(o => o.ByteArrayProperty, (o, rv) => o.ByteArrayProperty = rv, TestHelpers.GetBytes(5), TestHelpers.GetBytes(10));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I adopted the pattern of using Assert.That with Is.EqualTo().Using() with the RealmValueWithCollections.RealmValueComparer. I guess it wraps non-RealmValues in RealmValues, but considered it easier than abstracting the comparison just for this single case.

@rorbech rorbech requested a review from papafe April 8, 2024 07:13
Copy link
Contributor

@papafe papafe left a comment

Choose a reason for hiding this comment

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

Looks good to me! Just left a small comment

@papafe
Copy link
Contributor

papafe commented Apr 9, 2024

@nirinchev This is ready to merge for me if you have nothing against it 😁

@rorbech rorbech merged commit 9eaca6c into main Apr 9, 2024
79 of 81 checks passed
@rorbech rorbech deleted the cr/collections-in-mixed-sync branch April 9, 2024 11:57
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes no-changelog Used to skip the changelog check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants