elmcraft / core-extra

Utility functions for an improved experience with elm/core
https://package.elm-lang.org/packages/elmcraft/core-extra/latest/
Other
23 stars 11 forks source link

Add unionWith #19

Closed lydell closed 1 year ago

lydell commented 1 year ago

This is a port of https://github.com/elm-community/dict-extra/pull/29 by @SiriusStarr.

I created this PR by cherry-picking the commits from Sirius’ fork of dict-extra. Git is amazing sometimes.

Original PR description below. Note: It mentions two functions, but after discussion we decided to have just one.


unionWith is a pretty commonly used function, e.g. when counting occurrences, and unionWithKey is essentially free, since it's convenient to define unionWith in terms of it.

Dict.Extra.unionWith (+) (countOccurrencesIn a) (countOccurrencesIn b)

countOccurrencesIn : Expr -> Dict String Int
lydell commented 1 year ago

The only thing I wonder is if we need unionWith, since defining it with the other one is quite trivial, and pretty much all the Dict functions take the key by default (we don’t have mapWithKey, we just have map).

That’s a good point! I’d be fine with that. @SiriusStarr @jfmengels what do you say?

SiriusStarr commented 1 year ago

That’s a good point! I’d be fine with that. @SiriusStarr @jfmengels what do you say?

My personal take is that including functions is essentially free, and I personally prefer it explicitly stated whether the combining is dependent on the key or not, but if you are particularly worried about clutter in the documentation, then only unionWithKey could be included (as unionWith).

gampleman commented 1 year ago

I think in this case it’s clutter and consistency: why do all the other functions not have these two versions?

Another idea: for Basics.Extra we could introduce the dual to always, the function a -> b -> b (name TBD), which comes in handy pretty frequently and especially for Dict would be often quite handy.

gampleman commented 1 year ago

Also I should mention that I disagree with the claim that adding functions is essentially free. When reading code you need to understand what any particular function does in order to understand the containing code. In rare cases the function name is so obvious that you can correctly guess without much difficulty. But most of the time you need to know or find out what the function does and keep that information in your head. The more functions there are in a module, the harder that gets (and List.Extra is a good example - I find myself looking up the same functions over and over - there are so many).

So I think trivial variations do have a cost. Now in many cases the added convenience may totally dominate that cost, but I don’t think that should be assumed a forgone conclusion automatically.

SiriusStarr commented 1 year ago

When reading code you need to understand what any particular function does in order to understand the containing code. In rare cases the function name is so obvious that you can correctly guess without much difficulty.

This is more a failure of naming. I would hope that unionWith and unionWithKey fall into that category; if not, they should be renamed.

Regardless, it's my take, not a claim of any sort.

lydell commented 1 year ago

The function that have needed myself, is the one without key. Yet an alternative is to provide only the one without key and direct people to Dict.merge if they need it.

gampleman commented 1 year ago

So after discussing with other maintainers, I think we'd like to go for now with Dict.Extra.unionWith : (comparable -> a -> a -> a) -> Dict comparable a -> Dict comparable a -> Dict comparable a.

Happy to make the changes @lydell if you don't want to.

lydell commented 1 year ago

Updated!

Let me know if there’s something else you would like to see changed, or feel free to do so yourself.

gampleman commented 1 year ago

Thank you @lydell, @SiriusStarr and @jfmengels!