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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Count method #351

Closed
davydovanton opened this issue Nov 22, 2016 · 19 comments
Closed

Count method #351

davydovanton opened this issue Nov 22, 2016 · 19 comments
Assignees

Comments

@davydovanton
Copy link
Member

I think it will be awesome to create COUNT command for repository object. What do you think?
If you agree I can send PR 馃槉

@mereghost
Copy link
Member

Not sure if I follow the reasoning. ROM already provides a #count method. Keeping it private to the repository would remain consistent (besides I rarely needed a count without either a filter or an aggregation, but YMMV).

@davydovanton
Copy link
Member Author

/cc @hanami/core-team, @hanami/contributors, @solnic and @flash-gordon

@davydovanton
Copy link
Member Author

After a little discussion with @flash-gordon I want to update my point of view.

Of course I can call count method from rom relation object like this UserRepo.new.users.count. But I think it looks strange in hamani model. That's why I suggest adding count alias like where, find and create. And after that, it'll look like more understandable like UserRepo.new.count

@beauby
Copy link
Contributor

beauby commented Nov 24, 2016

@davydovanton Note that in the latest version of hanami-model there is no publicly available #where method.
In your suggestion, your count method would only return the count of all users, without scoping, right? If so, it would have little use IMO (since you can't combine it with where from the outside).

@davydovanton
Copy link
Member Author

@beauby yep, I think that #count should return only integer value

@beauby
Copy link
Contributor

beauby commented Nov 24, 2016

@davydovanton Right. What I meant was more: would you expect this Hanami::Repository#count method to take parameters?

@davydovanton
Copy link
Member Author

davydovanton commented Nov 24, 2016

@beauby good question. I can't answer this question. Now I want to see a simple implementation of #count method. Without params. But maybe someone wants to see something like rails #count with conditions and etc.

@beauby
Copy link
Contributor

beauby commented Nov 24, 2016

@davydovanton That's what I thought. The philosophy of the new hanami-model, though, is to not expose such query methods externally, and instead have you define intent-stating methods.

@flash-gordon
Copy link
Member

It probably can be done via blocks, like repo.count { active } so you can't detach a query object from a repo.

@jodosha
Copy link
Member

jodosha commented Nov 30, 2016

Please cast your vote here. What do you think about this feature?

It would be:

class BookRepository < Hanami::Repository
  def on_sale
    books.where(on_sale: true)
  end
end

repository = BookRepository.new
repository.count # => 100
repository.on_sale.count # => 12

@davydovanton
Copy link
Member Author

@jodosha awesome 馃憤

@beauby
Copy link
Contributor

beauby commented Nov 30, 2016

So basically it's an ActiveRecord scope? Because then you could do BookRepository.new.where(foo: :bar).combine(...).limit(5) BookRepository.new.on_sale.where(foo: :bar).combine(...).limit(5).

Basically, either you need the records, and you get no penalty from materializing the collection, or you don't need the records, and you might as well define an actual repository method for getting the count.

@solnic
Copy link
Member

solnic commented Nov 30, 2016

@beauby relation API is not exposed via repos so it'd be something like repo.books.where(...).count

@solnic
Copy link
Member

solnic commented Nov 30, 2016

FWIW I don't think repos should expose such a db-specific API as count. If you need this, just reach to a specific relation, ie repo.books.count. Having a Repo#count would be super hard to implement if it was supposed to work with any adapter.

@beauby
Copy link
Contributor

beauby commented Nov 30, 2016

@solnic My bad, I edited my answer (forgot the .on_sale method, to build upon @jodosha's example).

@jodosha
Copy link
Member

jodosha commented Dec 2, 2016

Having a Repo#count would be super hard to implement if it was supposed to work with any adapter.

I forgot about this aspect, thank you @solnic to reminding us.


Folks, I think this changes the direction of this discussion. We can't have #count as part of the public interface of Hanami::Repository, because count logic isn't necessary implemented by the underlying ROM adapter (and database).

@jodosha
Copy link
Member

jodosha commented Dec 2, 2016

Folks, I've implemented a proposal at #354, can you please have a look? Thanks.

@jodosha jodosha self-assigned this Dec 2, 2016
@jodosha jodosha added this to the v1.0.0.beta1 milestone Dec 23, 2016
@jodosha jodosha removed this from the v1.0.0.beta1 milestone Jan 3, 2017
@jodosha
Copy link
Member

jodosha commented Jan 3, 2017

Now that we have a more clear picture of both the implications and the solutions, I think we can close this.


"Count" is a concept not generally available to all the databases. SQL databases have it, but others don't.

If you're using a SQL database you can expose a method for that.

class BookRepository < Hanami::Repository
  def count
    books.count
  end
end

If you want to count the records, under specific conditions:

class BookRepository < Hanami::Repository
  # ...

  def on_sale_count
    books.where(on_sale: true).count
  end
end

If you want to use raw SQL you can do:

class BookRepository < Hanami::Repository
  # ...

  def old_books_count
    books.read("SELECT id FROM books WHERE created_at < (NOW() - 1 * interval '1 year')").count
  end
end

We should document this. I'm gonna create an issue over our website repository.

@jackbit
Copy link

jackbit commented Jan 8, 2017

wisely it should be external plugin / gem and not in core modules. It is classic issue in ActiveRecord at beginning and till now, we can not combine mongoose, mongoid, oracle or any else into the same active record, the solution is remove active record and use external plugin or create an enhanced adapter. It should be the same game for hanami.

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

No branches or pull requests

7 participants