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

support realm refresh() #1046

Merged
merged 26 commits into from
Jan 30, 2023
Merged

support realm refresh() #1046

merged 26 commits into from
Jan 30, 2023

Conversation

blagoev
Copy link
Contributor

@blagoev blagoev commented Nov 29, 2022

Add support for Realm.refresh() and onRefresh callbacks
#793

@coveralls
Copy link

coveralls commented Nov 29, 2022

Pull Request Test Coverage Report for Build 4044219192

  • 15 of 20 (75.0%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.006%) to 88.987%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/src/native/realm_core.dart 9 14 64.29%
Files with Coverage Reduction New Missed Lines %
lib/src/realm_class.dart 1 93.77%
Totals Coverage Status
Change from base Build 4042717374: 0.006%
Covered Lines: 3014
Relevant Lines: 3387

💛 - Coveralls

lib/src/realm_class.dart Outdated Show resolved Hide resolved
don't invoke the callback if the onRefresh registration fails
fix test for outside transaction registration
@blagoev blagoev marked this pull request as ready for review November 30, 2022 16:55
Copy link
Contributor

@desistefanova desistefanova left a comment

Choose a reason for hiding this comment

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

I have added some questions.

CHANGELOG.md Outdated Show resolved Hide resolved
test/realm_test.dart Outdated Show resolved Hide resolved
test/realm_test.dart Show resolved Hide resolved
test/realm_test.dart Show resolved Hide resolved
test/realm_test.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@nielsenko nielsenko left a comment

Choose a reason for hiding this comment

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

Mostly test related suggestions

lib/src/native/realm_core.dart Outdated Show resolved Hide resolved
lib/src/native/realm_core.dart Outdated Show resolved Hide resolved
test/realm_test.dart Outdated Show resolved Hide resolved
test/realm_test.dart Outdated Show resolved Hide resolved
test/realm_test.dart Outdated Show resolved Hide resolved
test/realm_test.dart Outdated Show resolved Hide resolved
@blagoev blagoev requested a review from nielsenko December 2, 2022 11:54
test/realm_test.dart Outdated Show resolved Hide resolved
@blagoev blagoev marked this pull request as draft December 2, 2022 13:34
@blagoev
Copy link
Contributor Author

blagoev commented Dec 2, 2022

blocked on realm/realm-core#6075

@desistefanova desistefanova marked this pull request as ready for review January 29, 2023 22:03
@desistefanova desistefanova self-assigned this Jan 29, 2023
Copy link
Contributor

@nielsenko nielsenko left a comment

Choose a reason for hiding this comment

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

I think we should consider dropping the synchronous version, and renaming the async version to just refresh

test/realm_test.dart Outdated Show resolved Hide resolved
@blagoev blagoev assigned blagoev and unassigned blagoev Jan 30, 2023
Copy link
Contributor Author

@blagoev blagoev left a comment

Choose a reason for hiding this comment

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

[CHANGES REQUESTED].
I can't not make my own PRs RED in GH

lib/src/realm_class.dart Outdated Show resolved Hide resolved
lib/src/native/realm_core.dart Show resolved Hide resolved
lib/src/realm_class.dart Outdated Show resolved Hide resolved
lib/src/realm_class.dart Outdated Show resolved Hide resolved
lib/src/realm_class.dart Outdated Show resolved Hide resolved
test/realm_test.dart Outdated Show resolved Hide resolved
test/realm_test.dart Show resolved Hide resolved
test/realm_test.dart Outdated Show resolved Hide resolved
test/realm_test.dart Outdated Show resolved Hide resolved
test/realm_test.dart Show resolved Hide resolved
Copy link
Contributor Author

@blagoev blagoev left a comment

Choose a reason for hiding this comment

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

[SUGGESTED CHANGES]

lib/src/realm_class.dart Outdated Show resolved Hide resolved
lib/src/realm_class.dart Outdated Show resolved Hide resolved
test/realm_test.dart Show resolved Hide resolved
@blagoev blagoev merged commit fe9d324 into main Jan 30, 2023
@blagoev blagoev deleted the blagoev/support-refresh branch January 30, 2023 14:33
desistefanova added a commit that referenced this pull request Feb 1, 2023
* support realm refresh()

* fix up changelog

* support refresh callback

* fix tests, add not called test

* make operation a no op if the callback was not registered successfully

don't invoke the callback if the onRefresh registration fails
fix test for outside transaction registration

* fix expectation

* fix refresh callback implementation.

* add refreshAsync to changelog

* use proper future timeout

* fix test

* Update realm in another Isolate

* Disable auto refresh realm and refresh manually

* Return result from refreshAsync

* Fix API doc

* Fix API doc

* Repair a test

* Code review changes

* Code review changes

* Code review changes

* Apply suggestions from code review

Co-authored-by: blagoev <lubo@blagoev.com>

* Code review changes

---------

Co-authored-by: Desislava Stefanova <dst.stefanova@gmail.com>
Co-authored-by: Desislava Stefanova <95419820+desistefanova@users.noreply.github.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants