gleam-lang / stdlib

🎁 Gleam's standard library
https://hexdocs.pm/gleam_stdlib/
Apache License 2.0
450 stars 162 forks source link

`list.count` and `iterator.count` #592

Closed apainintheneck closed 3 months ago

apainintheneck commented 3 months ago

This would function like Array#count does when it takes a block in Ruby.

irb(main):001:0> list = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
=> [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
irb(main):002:0> list.count { |x| x.odd? }
=> 5

In Gleam, this can obviously be hand-coded in a few different ways but this seems like something that happens often enough that it'd be nice to have a standard library function for it.

[0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
|> list.count(int.is_odd)
fn count(list: List(a), predicate: fn(a) -> Bool) -> Int
inoas commented 3 months ago

I'd assume this does what list.length does.

apainintheneck commented 3 months ago

Good point, maybe count_if would be a better name than count.

lpil commented 3 months ago

We don't ever use suffixes like _if. Count is fine, the types make it clearer.

thorhj commented 3 months ago

I'm moving the discussion in the PR here.

@lpil is debating whether iterator.count is necessary, since the functionality is easily replicated by

iter |> iterator.filter(f) |> iterator.length

For list.count there is a performance benefit, since it can be calculated with a single iteration using the more complex/general-purpose fold (as opposed to list.filter(f) |> list.length which requires two iterations), so list.count has more value.

However, I'm suggesting that there is also value in having the same functions available for iterator and list when possible, since users will become familiar with list.count, and it missing in iterator is a minor, confusing discrepancy.

I'll align the PR when a decision is reached 😄

chuckwondo commented 3 months ago

Just my 2 cents, but I agree that the consistency of having both list.count and iterator.count outweighs the fact that iterator.count isn't necessary from a performance perspective.

lpil commented 3 months ago

We don't aim to make the modules have the same API, rather we try and make it so the functions match if it makes sense for them both to have a function. If in future there is a clear need for the function then we can add it, but so far I've not had anyone ask for it. Just list please.

apainintheneck commented 3 months ago

I mentioned iterator.count at the beginning without realizing that iter |> iterator.filter(f) |> iterator.length was equivalent from a performance perspective and assuming that consistency was desired. Not including it makes sense to me now.

thorhj commented 3 months ago

Copy that, I have removed iterator.count from the PR now 🔥

ustitc commented 3 months ago

This function might be useful also for dict and set. Please have a look at the related PR: https://github.com/gleam-lang/stdlib/pull/617.

Thanks!

lpil commented 3 months ago

Maybe! Has it come up much in your code? What were you doing when it did? Thank you.

ustitc commented 3 months ago

🤔 I see your point.

I don't have any practical case for those functions; my motivation was driven by consistency. I aimed to create similar functions for all data structures.

And I totally missed your earlier message in this thread about prioritising need over consistency. I'll cancel the PR. Thanks for the feedback!

apainintheneck commented 3 months ago

Closing as this already got implemented by @thorhj!