serokell / universum

:milky_way: Prelude written in @Serokell
MIT License
174 stars 26 forks source link

`group`-like functions must return `NonEmpty` #250

Closed Martoon-00 closed 2 years ago

Martoon-00 commented 2 years ago

That's annoying that group returns [[a]] and not [NonEmpty a] - often working with the result requires an unsafe cast to NonEmpty. In the most common case, I want to write map head . group and I can't because head knows about NonEmpty and group does not.


However we need to think about whether do we want to change the existing group (thus causing breaking changes), or add a separate groupNE.

I vote for the breaking change variant because in some cases the next function in the pipeline is already polymorphic thanks to Universum (e.g. fmap length), in some cases this next function is unsafe (e.g. fmap Unsafe.head) and we want to change it. Changing the signature of group much more often should make the code cleaner than not.

Also, taking into account that recently we changed minimum and maximum to work with NonEmpty (breaking change that adds safety), I think changing group would not be a heavy unexpected change for the users.

dcastro commented 2 years ago

@int-index suggested re-exporting Data.List.NonEmpty.groupBy instead of Data.List.group, let's do that.