Skip to content

Prepare release 0.2.11.0 #266

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

Merged
merged 1 commit into from
Jun 17, 2020
Merged

Prepare release 0.2.11.0 #266

merged 1 commit into from
Jun 17, 2020

Conversation

sjakobi
Copy link
Member

@sjakobi sjakobi commented Jun 6, 2020

I'd like to wait with the release for another week in order to possibly include #246 and #259.


* Add `HashMap.findWithDefault` (deprecates `HashMap.lookupDefault`)
* Add `HashMap.findWithDefault` (soft-deprecates `HashMap.lookupDefault`).
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside: I'm fine with the addition of findWithDefault for compatibility with containers, but I'm wondering whether we really want to get rid of lookupDefault, which is arguably the nicer name.

Thoughts on this, @emilypi, @treeowl, @m-renaud?

The relevant commit is da5ac39.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the reason I initially went with findWithDefault is because it had precedence in several packages(hoogle=findWithDefault), including containers which is a "core" package (for some definition of core) as well as nonempty-containers, hashmap, enummapset, and others. lookupDefault also exists in the ecosystem (hoogle=lookupDefault) but fewer, and none in a "core" package.

I don't have a strong preference one way or the other, as long as there is consistency. imho if we decide to go with lookupDefault we should make sure its reflected in all the container-like packages.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I've said elsewhere, I'd be ever so much happier ignoring or deprecating both of them and using a more "eliminator-style" function like

lurkup :: (Hashable k, Eq k) => r -> (a -> r) -> k -> HashMap k a -> r

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lookup = lurkup Nothing Just
lurkup r f k = maybe r f . lookup k

Copy link
Collaborator

@m-renaud m-renaud Jun 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I agree this is more generally applicable, I have a hunch this will just result in everyone defining differently named versions of (and likely with different argument orderings of) lookupDefault a k m = lurkup a id k m, which will just cause more fragmentation. The idea of "looking up a value from some container, and returning a default value if its missing" is pretty ubiquitous and imo thus deserves a function. If we were to deprecate find/lookupDefault would we then require all code to replace it with a call to lurkup?

Edit: ....or will lead to someone creating a containers-missing package which just wraps containers and exports lookupDefault and any other common operations that are removed from containers to have a more general API.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's definitely no need to deprecate these things. But it seems strange to me to focus on adding more of them when we haven't yet added the more general idea. Note that while you need to import Data.Maybe to get fromMaybe and Data.Either to get fromLeft and fromRight, maybe and either are right in the Prelude. So is foldr. So this "eliminator style" is even more ubiquitous, and I'd say it's a high priority to get it in here and containers. We just need an appropriate name!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've thought about this some more (and regretted not making a proper issue).

  • I still think the addition of findWithDefault to achieve better consistency with containers is OK.
  • I realized that I do care slightly more about inter-package consistency than perfect naming, so the soft-deprecation of lookupDefault is also OK.
  • If I'd build the first containers-like package for Haskell, I'd probably add neither findWithDefault nor lurkup because both can easily be derived from lookup. These days, I also find that explicit pattern matching helps with readability, so I'd generally recommend using lookup and pattern matching on the result.

This adds and fixes some since annotations.
@sjakobi sjakobi force-pushed the release/0.2.11.0 branch from 2cb964c to 88b568a Compare June 13, 2020 12:26
@sjakobi
Copy link
Member Author

sjakobi commented Jun 13, 2020

Ready to release! :)

@treeowl, can you take it from here?

@sjakobi
Copy link
Member Author

sjakobi commented Jun 16, 2020

Ping @treeowl! :)

If you give me upload permissions, I could do the release too BTW!

@treeowl
Copy link
Collaborator

treeowl commented Jun 16, 2020

Sorry, let me do one more pass today and then I'll release.

@treeowl
Copy link
Collaborator

treeowl commented Jun 17, 2020

Oh, crud. I got mixed up and thought this was an issue rather than a PR, and started doing some of this, and I think also some other changelog updates, in a different commit. Can you please rebase carefully to make sure we don't double up?

@treeowl
Copy link
Collaborator

treeowl commented Jun 17, 2020

Never mind. It didn't push....

@treeowl treeowl merged commit 3411516 into master Jun 17, 2020
@treeowl
Copy link
Collaborator

treeowl commented Jun 17, 2020

@sjakobi sjakobi deleted the release/0.2.11.0 branch June 17, 2020 13:03
@sjakobi
Copy link
Member Author

sjakobi commented Jun 17, 2020

I've made a few more tweaks to the changelog in #269. I'll cut the release once that is merged.

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.

3 participants