Skip to content
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

Merged
merged 3 commits into from
Sep 11, 2023
Merged

Add unionWith #19

merged 3 commits into from
Sep 11, 2023

Conversation

lydell
Copy link
Contributor

@lydell lydell commented Sep 7, 2023

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, 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

SiriusStarr and others added 2 commits September 7, 2023 20:22
Doc comment accidentally referenced `unionWith` instead of `unionWithKey`

Co-authored-by: Jeroen Engels <[email protected]>
Copy link
Collaborator

@gampleman gampleman left a 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).

src/Dict/Extra.elm Outdated Show resolved Hide resolved
@lydell
Copy link
Contributor Author

lydell commented Sep 7, 2023

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
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Contributor Author

lydell commented Sep 8, 2023

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
Copy link
Collaborator

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 lydell changed the title Add unionWith and unionWithKey Add unionWith Sep 11, 2023
@lydell
Copy link
Contributor Author

lydell commented Sep 11, 2023

Updated!

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

@gampleman gampleman merged commit 4206074 into elmcraft:master Sep 11, 2023
2 checks passed
@gampleman
Copy link
Collaborator

Thank you @lydell, @SiriusStarr and @jfmengels!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants