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 unionsWith to combine a list of hash maps #118

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ppetr
Copy link

@ppetr ppetr commented Jan 22, 2016

No description provided.

@sjakobi sjakobi linked an issue May 31, 2020 that may be closed by this pull request
@sjakobi
Copy link
Member

sjakobi commented May 31, 2020

Uh, sorry for the late review… 😄

Probably a dumb question, by why doesn't this need both strict and lazy variants?

@sjakobi sjakobi mentioned this pull request Jun 18, 2020
@sjakobi
Copy link
Member

sjakobi commented Jun 18, 2020

Probably a dumb question, but why doesn't this need both strict and lazy variants?

To answer my question: It quite certainly does! We also have lazy and strict variants of unionWith.

Before any further work on the code is done here, we need to make a decision on the correct type for this function though: See #240 (comment).

@treeowl
Copy link
Collaborator

treeowl commented Jun 18, 2020

I continue to wonder if this function meets the Fairbairn threshold. It's an entirely immediate application of foldl' and unionWith.

@sjakobi
Copy link
Member

sjakobi commented Jun 18, 2020

I continue to wonder if this function meets the Fairbairn threshold. It's an entirely immediate application of foldl' and unionWith.

One argument for providing the function is that we could include proper documentation on the order in which the values are combined. unionWith unionsWith has the same problem as fromListWith, I believe.

@treeowl
Copy link
Collaborator

treeowl commented Jun 18, 2020

That's a problem for unionWith. If we don't provide unionsWith, then we certainly don't have to document it! At all! Huge win.

@sjakobi
Copy link
Member

sjakobi commented Jun 18, 2020

Sorry, I meant to say that unionsWith has a similar gotcha as fromListWith. That's an argument for holding off on unionsWith until we've made some progress on #264 though.

@sjakobi sjakobi removed a link to an issue Jun 20, 2020
@sjakobi sjakobi marked this pull request as draft June 24, 2020 10:58
@Prillan
Copy link

Prillan commented Dec 29, 2022

I continue to wonder if this function meets the Fairbairn threshold. It's an entirely immediate application of foldl' and unionWith.

Just a comment from a random passerby, I'd much prefer to have unions and unionsWith exposed by the library since I, a normal user without any intricate knowledge of the library, do not know which implementations would be best.

By not including the two functions you would leave it up to everyone to figure out whether foldl' union empty, fromList . concatMap toList or any other version is the most performant. The best implementation might not even use any exported functions!

Data/HashMap/Base.hs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants