-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add unionWith #19
Add unionWith #19
Conversation
Doc comment accidentally referenced `unionWith` instead of `unionWithKey` Co-authored-by: Jeroen Engels <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
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 |
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 |
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. |
This is more a failure of naming. I would hope that Regardless, it's my take, not a claim of any sort. |
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 |
So after discussing with other maintainers, I think we'd like to go for now with Happy to make the changes @lydell if you don't want to. |
Updated! Let me know if there’s something else you would like to see changed, or feel free to do so yourself. |
Thank you @lydell, @SiriusStarr and @jfmengels! |
This is a port of elm-community/dict-extra#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, andunionWithKey
is essentially free, since it's convenient to defineunionWith
in terms of it.