universum icon indicating copy to clipboard operation
universum copied to clipboard

Add special grouping functions

Open Martoon-00 opened this issue 2 years ago • 3 comments

Description

I often witnessed the need in [(a, b)] -> [(a, [b])] grouping function, so adding it here since we are working on similar things in-parallel.

Related issues(s)

✓ Checklist for your Pull Request

Ideally a PR has all of the checkmarks set.

If something in this list is irrelevant to your PR, you should still set this checkmark indicating that you are sure it is dealt with (be that by irrelevance).

  • [x] I made sure my PR addresses a single concern, or multiple concerns which are inextricably linked. Otherwise I should open multiple PR's.
  • [x] If your PR fixes/relates to an open issue then the description should reference this issue. See also auto linking on github.

Related changes (conditional)

  • Tests

    • [x] If I added new functionality, I added tests covering it.
    • [x] If I fixed a bug, I added a regression test to prevent the bug from silently reappearing again.
  • Documentation

    I checked whether I should update the docs and did so if necessary:

    • [x] README
    • [x] Haddock
  • Record your changes

    • [ ] I added an entry to the changelog if my changes are visible to the users and
    • [x] provided a migration guide for breaking changes if possible

Stylistic guide (mandatory)

  • [x] My commit history is clean (only contains changes relating to my issue/pull request and no reverted-my-earlier-commit changes) and commit messages start with identifiers of related issues in square brackets.

    Example: [#42] Short commit description

    If necessary both of these can be achieved even after the commits have been made/pushed using rebase and squash.

Martoon-00 avatar Apr 12 '22 16:04 Martoon-00

Additionally, I'd like to include groupByKeyOn—it's less general, but it avoids those explicit preconditions by leaning on Eq.

I also like how *On version would go without preconditions on the provided function.

My thought on not including it was that other packages seem to avoid the addition of *On version unless it does something special compared to passing mere on call (i.e. there is no groupOn f = groupBy ((==) 'on' f)).

Other examples: sortOn which has slightly different performance characteristics and non-existing Data.List.NonEmpty.groupOn.

So I think if we go with adding groupByKeyOn, we should be consistent and add mere groupOn too, but this is likely the work for a separate issue.

What do you think?

Martoon-00 avatar May 20 '22 13:05 Martoon-00

I would like to have it for all the things. Sorting is weird because of sortOn f vs. sortBy (compare `on` f). I don't know if any of the other common functions have both options. I don't think these ones do.

treeowl avatar Jun 01 '22 08:06 treeowl

Okay, added On versions for grouping, and sorting is out of scope of this issue.

@treeowl Requesting the final review :eyes:

Martoon-00 avatar Jun 22 '22 22:06 Martoon-00