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

Add gatherEquals function. #114

Merged
merged 9 commits into from
Oct 8, 2018
Merged

Add gatherEquals function. #114

merged 9 commits into from
Oct 8, 2018

Conversation

robinheghan
Copy link
Contributor

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
Copy link
Contributor

pzp1997 commented Sep 27, 2018

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
Copy link
Contributor Author

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
Copy link
Collaborator

Chadtech commented Sep 27, 2018

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
Copy link
Contributor Author

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
Copy link
Contributor

pzp1997 commented Sep 30, 2018

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
Copy link
Contributor Author

Updated to return (a, List a)

@pzp1997
Copy link
Contributor

pzp1997 commented Sep 30, 2018

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
Copy link
Collaborator

Chadtech commented Oct 2, 2018

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
Copy link
Contributor Author

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
Copy link
Contributor

pzp1997 commented Oct 4, 2018

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
Copy link
Collaborator

Chadtech commented Oct 8, 2018

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

@Chadtech Chadtech merged commit 55d1bce into elm-community:master Oct 8, 2018
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.

None yet

3 participants