Open ghiculescu opened 4 years ago
I like this proposal. It's kind of like the case with where_not
. I think there might be more variations of methods once the query is grouped, and we can add those to this special module.
There are a few things from the Documentation that I'd like to point out:
You can write code like this and it would still have to return a __GROUPED relation.
Student.group(:first_name).where(site_id: 43).count
From the documentation, I think the method return type will have to be T::Hash[T.untyped, Integer]
thought, because it depends on how many attributes you've groupped
If count is used with Relation#group for multiple columns, it returns a Hash whose keys are an array containing the individual values of each column and the value of each key would be the count.
Article.group(:status, :category).count => {["draft", "business"]=>10, ["draft", "technology"]=>4, ["published", "business"]=>0, ["published", "technology"]=>2}
sig { returns(T::Hash[String, Integer]) }
def count; end
ActiveRecord_Relation__GROUPED
re-define more methods than just count
.T.any(String, T::Array[String])
is ideal.:raise_hands:
On Mon, Feb 24, 2020 at 11:21 AM Alex Ghiculescu notifications@github.com wrote:
- True. We could fix this by having ActiveRecord_Relation__GROUPED re-define more methods than just count.
- You're correct; will update proposal.
— You are receiving this because you commented.
Reply to this email directly, view it on GitHub https://github.com/chanzuckerberg/sorbet-rails/issues/273?email_source=notifications&email_token=AAFH4AP2EIUAY6N6KSNNCX3REQM5LA5CNFSM4KYCCR7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMZF7YI#issuecomment-590503905, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFH4AKHMUGQ7M35M7RUH3TREQM5LANCNFSM4KYCCR7A .
https://api.rubyonrails.org/classes/ActiveRecord/Calculations.html#method-i-count
Person.count
returns an IntegerArticle.group(:status, :category).count
returns a hash of typeT::Hash[String, Integer]
Currently there's no sigs for
count
on an AR::Relation generated, it's left to other places (sorbet rbi generation / sorbet-typed) to define.However, rbis generated for relations do
include Enumerable
, which means that by defaultcount
is assumed to returnInteger
. See sorbet.run.My proposal
I'm not sure if this is a good idea yet, I'd like feedback.
Currently rbis we generate for models include this:
We could change it to this:
And also define this in the same rbi:
I've tested this code statically and it works great. I imagine it might create some runtime problems which is why I want feedback on it before pursuing the idea.