fix groupBy' name and groupBy semantics

[gregwebs]

as discussed, group' is better named groupAll

The problem with groupBy and groupAllBy is they don't have an Eq constraint. These functions just uses the first element in their comparison, meaning that equality must be transitive, which means there may as well be an Eq constraint. Here is a helper function to use with normal groupBy (like comparing) that adds an Eq: http://chrisdone.com/data-extra/Data-Eq-Extra.html

I have a version of groupBy that I use which does not require transitivity, this function rightly does not have Eq.

I suggest we fix the lack of Eq in the classy prelude. I am willing to add this patch.

[snoyberg]

I'm fine with the rename, but I'm confused by the rest of your comments. The types in mono-traversable are currently:

-- In IsSequence
groupBy' :: (Element c -> Element c -> Bool) -> c -> [c]
-- In EqSequence
group' :: c -> [c]

How would you want to change these signatures?

[gregwebs]

groupBy :: Eq b => (Element c -> b) -> c -> [c]

where the function will perform b == b

[snoyberg]

What's the advantage of that formulation? The current signature seems more canonical, and makes it easy to use arbitrary comparison functions.

[gregwebs]

The current signature isn't more canonical, Eq is the canonical way to do transitive equality, not Bool. Bool is certainly more flexible (but so are dynamic types).

The real problem is a mis-match between implementation & type signature. The Eq type signature I am giving matches the current implementation that requires transitivity.

I wrote my own groupBy of type (a -> a -> Bool) for a different use case that does not assume transitivity. The implementation actually steps through 2 elements at a time, whereas in the existing groupBy the first a in (a -> a -> Bool) is always the same. http://stackoverflow.com/questions/1316365/haskell-surprising-behavior-of-groupby

[snoyberg]

What I meant by canonical was that it's the normal way to do it, e.g. sortBy and groupBy.

But I've thought about what you've said, and realized you're absolutely right. I can't think of many examples of usage where your proposed change wouldn't be the easier way to write code. And for the few cases that come up, they demonstrate the problem you describe.

OK, I'll make the change.

[snoyberg]

Done.

[gregwebs]

great, thanks! This was something I was going to take to the Haskell libraries, but it is much better to put in classy-prelude first so I can report back to the libraries based on real experience.

I was thinking that if we want to supply type (a -> a -> Bool) we can call it groupWith (and it won't require transitivity). I have a slightly specialized version that I could probably cleanup. Only problem is there is a GHC.Exts groupWith that behaves differently, so maybe a different name is better.

[gregwebs]

@ekmett sent me this link as the original discussion of the groupBy signatures issue and wanted me to use the name sortOn rather than sortWith in Data.List.NonEmpty. http://lukepalmer.wordpress.com/2009/07/01/on-the-by-functions/

[snoyberg]

groupAll and groupAllOn sounds good to me, are you OK with the name change?

[gregwebs]

yeah, fine with me

[snoyberg]

OK, I've done the rename in mono-traversable.