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

Fix removeAt #884

Merged
merged 5 commits into from
Sep 6, 2022
Merged

Fix removeAt #884

merged 5 commits into from
Sep 6, 2022

Conversation

nielsenko
Copy link
Contributor

Apparently we never wired up and tested removeAt.

This fixes #883

@cla-bot cla-bot bot added the cla: yes label Sep 5, 2022
@nielsenko nielsenko force-pushed the kn/fix-list-removeAt branch from dea00fb to 805ac44 Compare September 5, 2022 13:02
@nielsenko nielsenko mentioned this pull request Sep 5, 2022
Closed
@nielsenko nielsenko marked this pull request as ready for review September 5, 2022 13:07
@coveralls
Copy link

Pull Request Test Coverage Report for Build 2993418403

  • 0 of 5 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+6.06%) to 92.891%
Totals Coverage Status
Change from base Build 2948644235: 6.06%
Covered Lines: 392
Relevant Lines: 422
💛 - Coveralls

@coveralls
Copy link

coveralls commented Sep 5, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 2999512875

  • 12 of 12 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.02%) to 85.818%
Totals Coverage Status
Change from base Build 2948644235: -1.02%
Covered Lines: 1652
Relevant Lines: 1925
💛 - Coveralls

@nielsenko nielsenko self-assigned this Sep 5, 2022
nirinchev
nirinchev reviewed Sep 5, 2022
View reviewed changes
Copy link
Member

@nirinchev nirinchev left a comment

Choose a reason for hiding this comment

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

This may be out of scope, but should we implement all the missing API for list?

As far as I can tell, we're lacking:

  • contains
  • set length -> should throw
  • removeWhere
  • retainWhere
  • removeLast
  • removeRange
  • replaceRange

@nielsenko
Copy link
Contributor Author

This may be out of scope, but should we implement all the missing API for list?

These are implemented by ListMixin, but yes we should strive to make them as efficient fro realm lists as possible. I would like to keep that separate from this PR though.

@nielsenko nielsenko requested a review from nirinchev September 5, 2022 14:13
@nirinchev
Copy link
Member

Isn't removeAt also implemented in ListMixin? The issue is that their implementation wouldn't work with Realm (the same way removeAt doesn't work) because they're manipulating the length of the list.

@nielsenko
Copy link
Contributor Author

Isn't removeAt also implemented in ListMixin? The issue is that their implementation wouldn't work with Realm (the same way removeAt doesn't work) because they're manipulating the length of the list.

Yes. I have implemented length setter to truncate, if newLength is larger than length, and implemented removeRange to support it.

I have also added tests for:

  • contains
  • set length -> should throw
  • removeWhere
  • retainWhere
  • removeLast
  • removeRange
  • replaceRange

@nielsenko
Implement length setter
e9d0b0c 
Length setter now truncates list, if newLength is less than current length.
Otherwise nothing happens.

Also removeRange implemented on top of removeAt as an optimization, since
ListMixins implementation does a lot of moving that is not needed by realm.
@nielsenko
Add tests for list interface on ManagedList
139de08 
The following methods are tested:
* contains
* set length -> truncates if new length is smaller
* removeWhere
* retainWhere
* removeLast
* removeRange
* replaceRange
@nielsenko
Add test that objects removed from lists still exists in realm
320412e
@nielsenko nielsenko force-pushed the kn/fix-list-removeAt branch from 606f13b to 320412e Compare September 6, 2022 08:31
@nielsenko nielsenko merged commit 42634e4 into master Sep 6, 2022
@nielsenko nielsenko deleted the kn/fix-list-removeAt branch September 6, 2022 11:26
@blagoev blagoev mentioned this pull request Sep 19, 2022
Closed
@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.
Reviewers

@nirinchev nirinchev nirinchev approved these changes

@desistefanova desistefanova Awaiting requested review from desistefanova

Assignees

@nielsenko nielsenko

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: List.removeAt doesn't work
4 participants
@nielsenko @coveralls @nirinchev @desistefanova