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 HashMap.findWithDefault #176

Merged

Conversation

m-renaud
Copy link
Collaborator

This function is equivalent to lookupDefault but uses the same name that the containers package uses Data.Map.Strict.findWithDefault.

This partially addresses #172.

@treeowl
Copy link
Collaborator

treeowl commented Jan 23, 2018 via email

@treeowl
Copy link
Collaborator

treeowl commented Jan 23, 2018

There is almost no benefit to doing this, and it breaks backwards compatibility. unordered-containers is not containers and isn't supposed to be. It has its own well-established names for things. Please focus instead on

  • Technical contributions: new functions that users can't easily write themselves, total or near-total versions of partial functions, new instances, and performance improvements. People really like performance improvements.

  • Documentation, but especially internal documentation. Ideally, developers should be able to get a pretty good sense of what's going on and how they can work on this package without having to read papers.

@treeowl treeowl closed this Jan 23, 2018
@m-renaud
Copy link
Collaborator Author

That's true, unordered-containers is not containers, but one would assume that their core APIs would be drop-in-replacement compatible. Is in the deprecation of the old function that you see as problematic, or the addition of the alias?

I think its a little strong to say that it breaks backwards compatibility, the old lookupDefault function still exists, the new one is just added for consistency with Data.Map.

@treeowl
Copy link
Collaborator

treeowl commented Jan 23, 2018 via email

@m-renaud
Copy link
Collaborator Author

Deprecation is breaking.

I guess in some sense since your code will now generate warnings when compiled, but not "breaking" in the sense of it not compiling anymore. In any case, I'm not attached to deprecating the old function, just thought I'd throw the idea out there :)

What makes you think the containers API is superior and should be followed?

I make no claims on which API is superior, but containers does ship with GHC and is considered a "core" package, so if one API was to be canonical I would lean towards containers.

Lots of packages use different names for similar things.

That's true, but a lot of packages aren't reasonable drop-in replacements, which would be my assumption for different "map" implementations. Unless I'm using functions specific to a certain implementation it would be nice to just change an import and have everything work.

In the case of Sequence and Vector they're different enough that one wouldn't assume that the APIs are exactly the same (although consistency is nice).

people go to the documentation to learn the local lingo

That is what they have to do now, but I would argue the less "local lingo" there is the better. Imagine if every type constructor that was a functor used a different name for fmap. I curse the Java libraries at work for this constantly, some use map, others use then, and others use transform which all have the type of our beloved fmap.

Predictable/consistent function naming make the experience of using a new package much smoother, and I don't think we should write-off making changes to improve consistency just because of how they got to this point. I do concede that I'm a newcomer to the Haskell ecosystem, and I'm not aware of all the history that exists or how difficult it is to deprecate something and make migration non-painful in this ecosystem. Would you be open to getting some input from others to see what their thoughts are? In the end its your decision, I'm just interested what the community thinks :)

@treeowl
Copy link
Collaborator

treeowl commented Jan 23, 2018 via email

@m-renaud
Copy link
Collaborator Author

Is this the only function with the same purpose and a different name or argument order or semantics?

As far as I'm aware, this is the only one. I went through the HashSet and HashMap Haddocks and everything else looks like its aligned. I'll send out a proposal to the libraries list when I get a chance.

@treeowl treeowl reopened this Jan 23, 2018
@treeowl
Copy link
Collaborator

treeowl commented Jan 23, 2018

Let's double check things like fromList bias, the order of the arguments to the fromListWith passed function, etc.

This function is equivalent to `lookupDefault` but uses the same name that
the containers package
uses (https://hackage.haskell.org/package/containers-0.5.10.2/docs/Data-Map-Strict.html#v:findWithDefault).

This partially addresses haskell-unordered-containers#172.
@m-renaud
Copy link
Collaborator Author

@m-renaud
Copy link
Collaborator Author

m-renaud commented Jan 4, 2019

Circling back to this, from the libraries proposal discussion linked above is that there were no strong objections to adding this method to HashMap. I'll admit that it didn't get as much input as I had hoped.

The only contentious point was how/when to deprecate the existing function due to the affect of on -Werror, especially since haskell/pvp#12 is still open. So, the safe way to proceed would be to update the function comment on lookupDefault to note that its deprecated but not "officially" deprecate it. I think the discussion on how to safely deprecate it is orthogonal to adding findWithDefault.

After I've resolved the merge conflicts can this be merged? Thanks!

Edit: Reworded first paragraph a little bit.

@m-renaud
Copy link
Collaborator Author

Hey @treeowl, I've resolved merge conflicts since I originally started this, and also opted for a "soft deprecation" via comments instead of a DEPRECATED pragma since its still isn't official that deprecating a function does not require a major version bump (although in this case its perfectly safe).

Let me know what I can do to move this along :)

@m-renaud
Copy link
Collaborator Author

Reviving this thread, can we merge this in? The PVP issue still hasn't been closed, so lets go with soft (aka. comment) deprecation for now until that is resolved.

@treeowl
Copy link
Collaborator

treeowl commented Mar 24, 2020

I still hate this because I still hate the function, let alone having two names for it. I'd much rather have something like

foolook :: (Hashable k, Eq k) => r -> (v -> r) -> k -> HashMap k v -> r
foolook r vr k hm = maybe r vr $ lookup k hm

but whatever.

@treeowl treeowl merged commit da5ac39 into haskell-unordered-containers:master Mar 24, 2020
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.

None yet

2 participants