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

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

Merged
merged 2 commits into from
Jul 25, 2018
Merged

Conversation

loganmac
Copy link
Contributor

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

pzp1997 commented May 31, 2018

See #49 for related discussion.

@Chadtech
Copy link
Collaborator

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

pzp1997 commented Jun 10, 2018

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

pzp1997 commented Jun 10, 2018

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

  • There is one other grouping function that should be updated for consistency, groupWhileTransitively.
  • There's a minor formatting issue on line 1311.
  • The version probably should not be bumped in a pull request.

@Chadtech
Copy link
Collaborator

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 Chadtech merged commit d712286 into elm-community:master Jul 25, 2018
@Chadtech
Copy link
Collaborator

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

@pzp1997
Copy link
Contributor

pzp1997 commented Jul 25, 2018

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

@Chadtech
Copy link
Collaborator

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

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

pzp1997 commented Aug 22, 2018

LGTM. Sorry for not getting to it sooner.

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.

3 participants