Skip to content

Implement bimapBoth (#174)#202

Merged
vrom911 merged 4 commits intokowainik:masterfrom
astynax:bimapBoth
Oct 4, 2019
Merged

Implement bimapBoth (#174)#202
vrom911 merged 4 commits intokowainik:masterfrom
astynax:bimapBoth

Conversation

@astynax
Copy link
Copy Markdown
Contributor

@astynax astynax commented Oct 3, 2019

Resolves #174

General

  • I've updated the CHANGELOG with the short description of my latest changes.
  • All new and existing tests pass.
  • I keep the code style used in the files I've changed (see style-guide for more details).
  • I've used the stylish-haskell file.
  • My change requires the documentation updates.
    • I've updated the documentation accordingly.
  • I've added the [ci skip] text to the docs-only related commit's name.

@chshersh chshersh requested review from chshersh and vrom911 October 3, 2019 19:30
@chshersh chshersh added the extra Relude.Extra label Oct 3, 2019
Copy link
Copy Markdown
Contributor

@chshersh chshersh left a comment

Choose a reason for hiding this comment

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

@astynax Thanks for your work! The implementation is clean, I have only a couple of suggestions regarding documentation.

[Left False,Right True]
>>> import Data.Functor.Const
>>> getConst $ bimapBoth (+ 1) (Const 41)
42
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's remove examples for triple and Const to decrease confusion 🙂 In most cases this function is going to be used with a pair or Either. So let's keep focus of documentation readers only on relevant examples. Experienced Haskell developers, who are familiar with Bifunctor, can generalise this function in their head.

secondF = fmap . second
{-# INLINE secondF #-}

{- | Maps a function over both elements of a bifunctor.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's move this function in before bimapF to be consistent with the order of functions in the export list.

@@ -16,7 +16,8 @@ qux :: Maybe (a, b)
-}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can add two more examples to the top-level docs:

xxx :: (a, a)
yyy :: Either a a

-}
mapBoth :: (a -> b) -> (a, a) -> (b, b)
mapBoth f (a1, a2) = (f a1, f a2)
{-# DEPRECATED mapBoth "Use Relude.Extra.Bifunctor.bimapBoth instead" #-}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor reformatting to bring focus to relevant parts

Suggested change
{-# DEPRECATED mapBoth "Use Relude.Extra.Bifunctor.bimapBoth instead" #-}
{-# DEPRECATED mapBoth "Use 'bimapBoth' from "Relude.Extra.Bifunctor" instead" #-}

Copy link
Copy Markdown
Contributor

@chshersh chshersh left a comment

Choose a reason for hiding this comment

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

Thanks, looks great!

Copy link
Copy Markdown
Member

@vrom911 vrom911 left a comment

Choose a reason for hiding this comment

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

Nice! Thank you 👍

@vrom911 vrom911 merged commit 10a5a78 into kowainik:master Oct 4, 2019
@astynax astynax deleted the bimapBoth branch October 5, 2019 17:34
@chshersh chshersh added the Hacktoberfest https://hacktoberfest.digitalocean.com/ label Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

extra Relude.Extra Hacktoberfest https://hacktoberfest.digitalocean.com/

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Generalise mapBoth to Bifunctor

3 participants