Skip to content

Add RealmList.removeAllFromRealm and Realm.clear#2061

Merged
beeender merged 1 commit intomasterfrom
mc/feature/1560
Jan 15, 2016
Merged

Add RealmList.removeAllFromRealm and Realm.clear#2061
beeender merged 1 commit intomasterfrom
mc/feature/1560

Conversation

@beeender
Copy link
Copy Markdown
Contributor

  • Add LinkView.removeAllTargetRows.
  • Add Realm.removeAll.
  • Add Realm.remove(RealmObject).
  • Add Realm.remove(Iterable).
  • Javadoc & test case update.

Close #1560

@beeender
Copy link
Copy Markdown
Contributor Author

@realm/java

Comment thread changelog.txt Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How about rearranging items.

  * Added RealmQuery.isNotEmpty() (#2025). (Thank you @stk1m1)
  * Added Realm.remove(RealmObject), Realm.remove(Iterable) and Realm.removeAll() (#1560).
  * Improved performance of RealmList#contains() (#897).

@nhachicha
Copy link
Copy Markdown
Collaborator

what happens when a pending insert transaction is running in the background. I guess removeAll doesn't guarantee everything will be erased ( that's the user responsibility to cancel async transac & make sure the call return before calling removeAll

@beeender
Copy link
Copy Markdown
Contributor Author

@nhachicha Yep, i think so. removeAll still needs to be called in the transaction and it will be user's responsibility to ensure the sequence of inserting and clearing.

@beeender beeender force-pushed the mc/feature/1560 branch 2 times, most recently from 4621881 to 504b146 Compare January 12, 2016 12:44
@beeender beeender changed the title Add removeAll and remove to Realm Add RealmList.removeAllFromRealm and Realm.clear Jan 12, 2016
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not use the constants CLASS_CAT and CLASS_DOG?

@beeender beeender force-pushed the mc/feature/1560 branch 2 times, most recently from 2c4aeaa to 55be9a1 Compare January 13, 2016 09:55
@cmelchior
Copy link
Copy Markdown
Contributor

@beeender Just noticed we have a Realm.clear(class). Should we rename clear() to clearAll() instead or is clear() and clear(class) good enough?

@beeender
Copy link
Copy Markdown
Contributor Author

@cmelchior I don't have strong opinion on both names. We might be able to have another function which is close to the word clear is clear everything, objects and schemas. Maybe that fits clearAll better? Currently it is easy for user to figure out the clear() is just clear all objects because of clear(String classname) is there. Right? Any other good reason that we shall use clearAll here?

@cmelchior
Copy link
Copy Markdown
Contributor

clear() fits the List API better, while clearAll() is a perhaps a bit easier to read if you don't know the method exists on the List interface. I think I'm fine with clear(), I just noticed it when doing something else.

@beeender
Copy link
Copy Markdown
Contributor Author

retest this please and god bless me

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What does this test?

  • IllegalStateException will be thrown if the RealmList is not managed.
  • IllegalStateException will be thrown if the RealmList contains standalone object.

I think we should split this into two tests.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

oh, testRealmRemoveAllNotManagedList already exists and it does not throw the IllegalStateException.

so, this tests IllegalStateException will be thrown if the RealmList contains standalone object.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  • "IllegalStateException will be thrown if the RealmList is not managed." This doesn't throw if every object is managed.
    See testRealmRemoveAllNotManagedList
  • "IllegalStateException will be thrown if the RealmList contains standalone object." This is what this test do.
  • A standalone object in a managed RealmList is not possible, so no test case for it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants