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.

Sign up for GitHub

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

Jump to bottom

RealmResultsChanges.isCleared #1265

Merged
merged 7 commits into from
May 25, 2023
Merged

RealmResultsChanges.isCleared #1265

merged 7 commits into from
May 25, 2023

Conversation

desistefanova
Copy link
Contributor

@desistefanova desistefanova commented May 5, 2023 •
edited
Loading

Deprecate isCleared for RealmResults.
Fixes #1189
Related to #1278

@desistefanova desistefanova added the no-changelog Used to skip the changelog check label May 5, 2023
@cla-bot cla-bot bot added the cla: yes label May 5, 2023
@coveralls
Copy link

coveralls commented May 5, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 5076610426

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.7%) to 88.614%
Totals Coverage Status
Change from base Build 5070997253: -0.7%
Covered Lines: 2654
Relevant Lines: 2995
💛 - Coveralls

@desistefanova desistefanova requested review from blagoev and nielsenko May 5, 2023 08:27
nirinchev
nirinchev requested changes May 5, 2023
View reviewed changes
Copy link
Member

@nirinchev nirinchev left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This doesn't seem correct to me. You can convert a list to results and subscribe for notifications. I would expect that to emit isCleared correctly when the underlying list is cleared.

@desistefanova
Copy link
Contributor Author

This doesn't seem correct to me. You can convert a list to results and subscribe for notifications. I would expect that to emit isCleared correctly when the underlying list is cleared.

Ok, I will revert these changes and will add a new test. Thank you for noticing this @nirinchev!

@desistefanova desistefanova marked this pull request as draft May 9, 2023 11:50
@desistefanova desistefanova changed the title Remove isCleared property from RealmResultsChanges RealmResultsChanges.isCleared May 9, 2023
@desistefanova desistefanova changed the base branch from main to ds/core_upgrade_13.8.0 May 9, 2023 12:33
@desistefanova desistefanova changed the base branch from ds/core_upgrade_13.8.0 to main May 9, 2023 12:40
@desistefanova desistefanova requested a review from nirinchev May 16, 2023 09:01
@desistefanova desistefanova marked this pull request as ready for review May 16, 2023 09:01
nielsenko
nielsenko approved these changes May 16, 2023
View reviewed changes
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.

LGTM

blagoev
blagoev reviewed May 22, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@desistefanova @blagoev
Update CHANGELOG.md
3448b8f 
Co-authored-by: blagoev <lubo@blagoev.com>
@desistefanova desistefanova removed the no-changelog Used to skip the changelog check label May 23, 2023
@desistefanova desistefanova merged commit 3065a3a into main May 25, 2023
@desistefanova desistefanova deleted the remove_results_is_cleared branch May 25, 2023 06:12
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@blagoev blagoev blagoev approved these changes

@nielsenko nielsenko nielsenko approved these changes

@nirinchev nirinchev Awaiting requested review from nirinchev

Assignees

@desistefanova desistefanova

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deprecate RealmResultsChanges.isCleared
5 participants
@desistefanova @coveralls @nirinchev @blagoev @nielsenko