search

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

Change group/groupWhile to use a tuple to represent a non-empty list #103

Closed loganmac closed 6 years ago

loganmac commented 6 years ago

Quote from @evancz in the slack:

Someone mentioned that using List.Extra.groupWhile is annoying because you are guaranteed to get at least one entry in every single list, but you then have to pattern match and handle the empty case anyway. Can it be changed to the following type?

groupWhile : (a -> a -> Bool) -> List a -> List (a, List a)

I am going to be emphasizing to people that “non-empty lists” are not hard to represent, no special library needed. I think doing it as a tuple is a fine way

This PR does just that.

pzp1997 commented 6 years ago

See #49 for related discussion.

Chadtech commented 6 years ago

Discussion seems to have fizzled out on this. To move forward, how about we merge this, in conjunction with a handful of non empty list helper functions, such as..

mapNonEmpty : (a -> b) -> (a, List a) -> (b, List b)
toNonEmpty : List a -> Maybe (a, List a)
fromNonEmpty : (a, List a) -> List a

..is that a good plan?

pzp1997 commented 6 years ago

I'm kind of against that proposal. At the end of the day we'll have a whole lot of duplicate functions just for dealing with non-empty lists that will pollute the library (you mentioned 3 or 4 functions in particular but why should we stop there?). Maybe this stuff belongs in a different package or module. Also, FYI toNonEmpty is the same thing as uncons.

pzp1997 commented 6 years ago

If we do decide to merge this PR, which again I don't think we should do, some work needs to be done.

Chadtech commented 6 years ago

We talked about this in the issues and came to the conclusion that we should merge this without any helpers. If anyone needs any helpers later we can always add them.

Chadtech commented 6 years ago

Oh, and I will publish this with the next major update, which I would estimate will be within a month or two.

pzp1997 commented 6 years ago

Please make sure the comments I made above are addressed before publishing.

Chadtech commented 6 years ago

Right, forgot about that, sorry. I will write that down and make sure those items are in the next release. I am going to have to sit down and solve some merge conflicts for the next release anyway. Unless @loganmac wants to do it (no pressure tho).

Chadtech commented 6 years ago

Okay, I have implemented those remaining changes in the 0.19 branch.

Rather than update groupWhileTransitively I just deleted it. I went to update it, but that involved figuring out what it did exactly compared to groupWhile. Then I found #45 , and realized the distinction was in dispute. I agreed with the opinions expressed in that issue we should just delete groupWhileTransitively, and so thats what I did.

Here is a demo: https://ellie-app.com/32KL9L2NtPKa1

Im worried this is too rushed. Its just some code I wrote up and pushed. So if you have any review of it I would appreciate it.

pzp1997 commented 6 years ago

LGTM. Sorry for not getting to it sooner.