elm-community / list-extra

Convenience functions for working with List.
http://package.elm-lang.org/packages/elm-community/list-extra/latest
MIT License
135 stars 59 forks source link

Add gatherEquals function. #114

Closed robinheghan closed 6 years ago

robinheghan commented 6 years ago

This is similar to group, except it groups all equal elements in a single sub-list. I'm open to change the name to something better.

pzp1997 commented 6 years ago

I really like the idea of this function but I'm wondering how useful this is without allowing for a custom equality function. It seems to me that the only useful thing you can do with the output is to calculate how many copies of each element there are. (Please tell me if I am overlooking something here.)

In that case maybe List (a, Int) would be a better return type for gatherEquals? We could also add a variant gatherEqualsWith : (a -> a -> Bool) -> List a -> List (a, List a) where the (a, List a) is to show that the sublists are non-empty.

robinheghan commented 6 years ago

We actually use it in production ^^

It's both for counting the number of things, but we also give each element an index and render each section together. So it does have more uses then just counting them.

Your arguments are valid, so I'm open to adding both gatherEqualsWith and gatherEqualsBy :)

Chadtech commented 6 years ago

How exactly are you using it?

I think I can kind of see how you might be using it. Something like List.repeat 5 xView would work, but you already have a list [ x, x, x, x, x ] so its easier to render all the items in the list, rather than count them and render that number of xViews. Like, an adventure video game where you have an inventory, and you might have 10 potions that are identical data with identical views. It would make more sense to just render all 10 potions since thats the data available to you then it would be to count the potions and do List.repeat potionCount potionView

Is that right? Am I anywhere close?

I also agree about adding gatherEqualsWith and gatherEqualsBy. Seems like a good idea to me.

robinheghan commented 6 years ago

Added gatherEqualsBy and gatherEqualsWith.

The way we use it is that we get a list of purchased tickets from our backend. We want to summarize what you've bought (eg. 2 adult tickets, 1 child ticket), and if the user clicks "view details" we want to print a human readable description of each ticket as well.

So while @pzp1997 would work, we would likely need to use List.repeat to render the detailed view later, or just render the backend provided list.

For us, grouping the elements then counting and rendering seemed like the better option.

pzp1997 commented 6 years ago

I can live with your desire to avoid List.repeat in the view function. How do we feel about making the return type List (a, List a) to reflect that the groups are guaranteed to be non-empty? (See #49 and #105 for related discussions about the place of non-empty in List.Extra.)

robinheghan commented 6 years ago

Updated to return (a, List a)

pzp1997 commented 6 years ago

I have a backlog of work right now but I can review your updated code in a couple days if no one gets to it first.

Chadtech commented 6 years ago

The code looks good to me. I havent run the tests yet to be 100% certain.

I do notice two things in the documentation

  1. The gatherEqualsBy example uses .name but all the records contain { age = .. }.
  2. The gatherEqualsWith example uses the function gatherEquals, but is otherwise correct.

Also, its just occurring to me that the names gatherEqualsBy and gatherEqualsWith could be confusing. For example, you could write a comparison function that returns True if two Int are not more different than 1, grouping 1 and 2 together, even tho 1 and 2 arent equal. What do you all think about gatherBy and gatherWith?

robinheghan commented 6 years ago

I agree with gatherWith. However, gatherEqualsBy should remain the same, as it provides an equality check on whatever the function returns, it doesn't allow the user to implement custom equality checking.

pzp1997 commented 6 years ago

I think you need to add gatherEqualsBy and gatherWith to the \@docs list. Also might be worth adding one sentence about the order in which the groups are returned and the order of the elements within each group. Other than that LGTM.

Chadtech commented 6 years ago

Great. Looks good to me too. Thanks @Skinney !