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

Generalise mapBoth to Bifunctor #174

Closed
chshersh opened this issue Jul 24, 2019 · 7 comments · Fixed by #202
Closed

Generalise mapBoth to Bifunctor #174

chshersh opened this issue Jul 24, 2019 · 7 comments · Fixed by #202
Labels
enhancement New feature or request extra Relude.Extra Hacktoberfest https://hacktoberfest.digitalocean.com/

Comments

@chshersh
Copy link
Contributor

Currently we have the following mapBoth function in the Relude.Extra.Tuple module:

mapBoth :: (a -> b) -> (a, a) -> (b, b)
mapBoth f (a1, a2) = (f a1, f a2)

I propose to generalise the type signature of this function so it can work with any Bifunctor:

mapBoth :: Bifunctor f => (a -> b) -> f a a -> f b b

For this we need:

  • Move mapBoth to Relude.Extra.Bifunctor
  • Generalise and implement
  • Add more examples to the documentation
@chshersh chshersh added enhancement New feature or request extra Relude.Extra labels Jul 24, 2019
@chshersh chshersh added this to the v0.6.0.0: Refinement milestone Jul 24, 2019
@vrom911 vrom911 added the Hacktoberfest https://hacktoberfest.digitalocean.com/ label Sep 29, 2019
@chshersh
Copy link
Contributor Author

Instead of moving, let's introduce bimapBoth function and deprecate mapBoth first.

@josephcsible
Copy link
Contributor

Should we do traverseBoth -> bitraverseBoth and generalize/move it too?

@chshersh
Copy link
Contributor Author

chshersh commented Oct 1, 2019

@josephcsible I'm not sure about bitraverseBoth. I would like to keep relude minimal and not introduce rarely used combinators. I needed mapBoth enough times to be included in Extra modules but I never needed bitraverseBoth

@josephcsible
Copy link
Contributor

We already have traverseBoth though, so we'd just be generalizing it, not adding a new one.

@chshersh
Copy link
Contributor Author

chshersh commented Oct 1, 2019

@josephcsible This involves solving several awkward problems:

  1. Deprecation cycle for traverseBoth should be done as well.
  2. bitraverse is for Bitraversable while the module is called Bifunctor. It feels awkward to put Bitraversable function into a module called Bifunctor. And introduce module with the name Bitraversable looks like a heavy solution for generalising one rarely used function. I would love to have fewer modules, not more 🙂

@astynax
Copy link
Contributor

astynax commented Oct 2, 2019

I can take this one, but what I'll need to do specifically? :)

Instead of moving, let's introduce bimapBoth function and deprecate mapBoth first.

Just this part?

@chshersh
Copy link
Contributor Author

chshersh commented Oct 2, 2019

@astynax Yep, only this part. Specifically:

  1. Implement bimapBoth in the Relude.Extra.Bifunctor module
  2. Add {-# DEPRECATE #-} pragma to mapBoth
  3. Move documentation from mapBoth to bimapBoth and add couple extra usage examples using doctest (they will be very helpful!)

@astynax astynax mentioned this issue Oct 3, 2019
7 tasks
vrom911 pushed a commit that referenced this issue Oct 4, 2019
* Implement `bimapBoth` (#174)

* fix style

* notes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request extra Relude.Extra Hacktoberfest https://hacktoberfest.digitalocean.com/
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants