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

Rename and deprecate mapToFst/Snd #196

Closed
vrom911 opened this issue Sep 22, 2019 · 4 comments · Fixed by #289
Closed

Rename and deprecate mapToFst/Snd #196

vrom911 opened this issue Sep 22, 2019 · 4 comments · Fixed by #289
Assignees
Labels
extra Relude.Extra question Further information is requested

Comments

@vrom911
Copy link
Member

vrom911 commented Sep 22, 2019

In #194 I've noticed that functions mapToFst/mapToSnd/mapBoth have a bit misleading name. Check out the type:

mapToFst :: (a -> b) -> a -> (b, a)

when I would expect it to have the following type:

mapToFst :: (a -> b) -> f a -> f (b, a)

(which is exactly what was needed in the mentioned PR, and we used map (mapToFst f) in there).

I also understand, that this is breaking change and I am not sure if it worth it. Need to thing about renaming or deprecating process a bit harder.

Anyway, at least adding functions which would actually map the containers is also a possible solution for the issue. I would really appreciate the discussion of this problem.

cc @kowainik/dev

@vrom911 vrom911 added question Further information is requested extra Relude.Extra labels Sep 22, 2019
@chshersh
Copy link
Contributor

@vrom911 I think renaming to less confusing names worth it in the long term. Regarding deprecation process: we can use the process described in my blog post (which includes multi-stage process with DEPRECATED pragmas and custom type errors):

Regarding mapBoth: we have a separate issue #174. Probably instead of only generalisation we can also rename the function to bimapBoth and have bimapBothF in the spirit of other functions in the Relude.Extra.Bifunctor module.

We can introduce new functions first and deprecate the old ones:

??? :: (a -> b) -> a -> (b, a)
fmapToFst :: Functor f => (a -> b) -> f a -> f (b, a)  -- uses ???

It's hard to come up with the good name for mapToFst... I can propose the following options:

  • withFst
  • mapKeepFst/keepFst
  • toFst

What do you think?

@willbasky
Copy link
Collaborator

Map implicates some f, therefore it is better to abandon it.
withFst i vote from you polling.

@josephcsible
Copy link
Contributor

I'm not a fan of renaming. In my mind, it makes sense that map in mapTo* refers to mapping over the first/second half of the tuple (and similarly for traverse). For example, note the following identities:

  • mapToFst f = bimap f id . dup = first f . dup
  • mapToSnd f = bimap id f . dup = second f . dup = fmap f . dup
  • mapBoth f = bimap f f
  • traverseToFst f = bitraverse f pure . dup
  • traverseToSnd f = bitraverse pure f . dup = traverse f . dup
  • traverseBoth f = bitraverse f f

Also, if you did rename the map* functions, what would it even make sense to rename the traverse* functions too?

@chshersh chshersh added this to the v0.7.0.0: Refiner milestone Apr 20, 2020
@chshersh
Copy link
Contributor

chshersh commented Apr 20, 2020

I propose the following change:

  1. Rename mapToFst and mapToSnd to toFst and toSnd correspondingly.
  2. Change the type of mapToFst (and mapToSnd) to the following:
fmapToFst :: Functor f => (a -> b) -> f a -> f (b, a)
fmapToSnd :: Functor f => (a -> b) -> f a -> f (a, b)
  1. Deprecate mapToFst and mapToSnd.

Now we have [fmap|traverse]to[Fst|Snd] and a sound naming scheme.

@vrom911 vrom911 self-assigned this May 12, 2020
@vrom911 vrom911 changed the title [RFC] rename mapToFst Rename and deprecate mapToFst/Snd May 12, 2020
@chshersh chshersh removed this from the v0.7.0.0: Refiner milestone May 12, 2020
@chshersh chshersh self-assigned this May 13, 2020
@chshersh chshersh added this to the v0.7.0.0: Refiner milestone May 13, 2020
chshersh added a commit that referenced this issue May 13, 2020
chshersh added a commit that referenced this issue May 13, 2020
vrom911 pushed a commit that referenced this issue May 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extra Relude.Extra question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants