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

implemented .count(), .isEmpty(), .removeAll() #42

Merged
merged 2 commits into from
Oct 21, 2019
Merged

implemented .count(), .isEmpty(), .removeAll() #42

merged 2 commits into from
Oct 21, 2019

Conversation

liquidiert
Copy link
Contributor

This PR addresses Issue #36.

  • implemented .count() and .countMax() functions from C api -> created new bindings
  • implemented .isEmpty() function from C api -> created new binding
  • implemented .removeAll() [remove all objects in box, not all ids from iterable -> still needs to be implemented?] function from C api -> created new binding

@liquidiert
Copy link
Contributor Author

Hey guys, it's my first PR ever, so please don't destroy it that much 😄 also advice is much appreciated :)

@vaind vaind changed the base branch from master to dev October 21, 2019 10:30
Copy link
Contributor

@vaind vaind left a comment

Choose a reason for hiding this comment

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

Thanks for the (your very first) PR 🥇

I had a couple of requests. Also, I've changed the target to be the dev branch, where all the feature branches get merged in first, before getting ready for a release. It's because we currently do one more review before the final merge into master. How this affects contributors: when implementing something, start from a dev branch, create a feature branch, make PR from your feature branch into the dev branch. Something like the git-flow described here: https://www.atlassian.com/git/tutorials/comparing-workflows/gitflow-workflow
Of course, there's no need for that now that this PR is already open. You can just commit the requested changes in your master as you've already started and they'll show up in the PR so we'll be able to merge them into dev.

lib/src/box.dart Outdated Show resolved Hide resolved
lib/src/box.dart Outdated Show resolved Hide resolved
test/box_test.dart Outdated Show resolved Hide resolved
@liquidiert
Copy link
Contributor Author

liquidiert commented Oct 21, 2019

I have two questions regarding the .remove() method:

  • shall it work only for a single id or also for iterables of ids?
  • and if you remove an id that has been removed before, ObjectBox returns a 404 exception -> do we need to handle that exception or is it intended that way?
    thanks for the fast answers btw :)

@vaind
Copy link
Contributor

vaind commented Oct 21, 2019

I have two questions regarding the .remove() method:

  • shall it work only for a single id or also for iterables of ids?

Thanks for the reminder. We have RemoveMany() in Go. How would you implement box.remove() for two types while keeping it statically typed? labeled parameters? Maybe it makes sense to have a separate method in Dart as well, since obx_box_remove_many() returns a count of removed IDs (as opposed to failing when removing a single one).

  • and if you remove an id that has been removed before, ObjectBox returns a 404 exception -> do we need to handle that exception or is it intended that way?

What would be an idiomatic way to handle this in Dart?

thanks for the fast answers btw :)
you're welcome :)

@liquidiert
Copy link
Contributor Author

I would've implemented it with type dynamic and a type check, but if we already have a different method that's probably better

@liquidiert
Copy link
Contributor Author

I don't know either unfortunately :/ that's why I've asked but I at least could wrap it with a more meaningful exception message than ObjecBoxException 404

@vaind
Copy link
Contributor

vaind commented Oct 21, 2019

I don't know either unfortunately :/ that's why I've asked but I at least could wrap it with a more meaningful exception message than ObjecBoxException 404

In that case, let's change to bool result and return false for this specific exception (not-found). Other exceptions, if any, should still be propagated.

@liquidiert
Copy link
Contributor Author

I would've implemented it with type dynamic and a type check, but if we already have a different method that's probably better

so are we going with the two methods approach?

@liquidiert
Copy link
Contributor Author

Ok rethought about the approaches and went with two methods 👍

@vaind vaind merged commit 7758a14 into objectbox:dev Oct 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants