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

Add RealmResults.isValid #1231

Merged
merged 3 commits into from
May 25, 2023
Merged

Add RealmResults.isValid #1231

merged 3 commits into from
May 25, 2023

Conversation

nielsenko
Copy link
Contributor

@nielsenko nielsenko commented Mar 28, 2023 •
edited
Loading

  • Add RealmResults.isValid
  • Add tests
  • Update CHANGELOG

Fixes: #1227

@cla-bot cla-bot bot added the cla: yes label Mar 28, 2023
@coveralls
Copy link

coveralls commented Mar 28, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 5077490219

  • 5 of 5 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 88.533%
Totals Coverage Status
Change from base Build 5077471942: 0%
Covered Lines: 2656
Relevant Lines: 3000
💛 - Coveralls

@nielsenko nielsenko force-pushed the kn/results-is-valid branch 2 times, most recently from 16f4a97 to 9ce47d9 Compare March 29, 2023 15:07
@nielsenko nielsenko changed the base branch from main to ds/core_upgrade_13.8.0 March 29, 2023 21:29
@nielsenko nielsenko marked this pull request as ready for review March 29, 2023 21:31
blagoev
blagoev approved these changes Mar 30, 2023
View reviewed changes
test/results_test.dart Outdated
expect(() => players.isEmpty, throwsException);
expect(playersAsResults.isValid, isFalse);
expect(() => playersAsResults, throwsException);
expect(() => playersAsResults.freeze(), throwsException);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we mark this with a TODO that we will be fixing it not to crash but return an empty list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Core reversed their thinking on this and decided it should throw, but it is consistent with isValid returning false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And now they reversed their thinking again..

@nielsenko nielsenko changed the base branch from ds/core_upgrade_13.8.0 to main May 1, 2023 08:59
@nielsenko nielsenko force-pushed the kn/results-is-valid branch 2 times, most recently from 42ee688 to 6a01dfe Compare May 15, 2023 09:41
@nielsenko nielsenko changed the base branch from main to kn/update-core May 25, 2023 06:56
Base automatically changed from kn/update-core to main May 25, 2023 07:54
@nielsenko nielsenko merged commit 188db09 into main May 25, 2023
@nielsenko nielsenko deleted the kn/results-is-valid branch May 25, 2023 09:01
@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

@desistefanova desistefanova desistefanova approved these changes

Assignees

@nielsenko nielsenko

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support RealmResults.isValid
4 participants
@nielsenko @coveralls @blagoev @desistefanova