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

Fix for #1297 #1334

Closed
wants to merge 7 commits into from
Closed

Fix for #1297 #1334

wants to merge 7 commits into from

Conversation

mgttlinger
Copy link
Contributor

We now have the problem of two map2 methods with different signatures.

@codecov-io
Copy link

codecov-io commented Aug 27, 2016

Current coverage is 91.39% (diff: 95.16%)

Merging #1334 into master will increase coverage by <.01%

@@             master      #1334   diff @@
==========================================
  Files           237        237          
  Lines          3567       3568     +1   
  Methods        3502       3504     +2   
  Messages          0          0          
  Branches         64         63     -1   
==========================================
+ Hits           3260       3261     +1   
  Misses          307        307          
  Partials          0          0          

Sunburst

Powered by Codecov. Last update 41d9ef8...9871ac8

@non
Copy link
Contributor

non commented Aug 30, 2016

Thanks @mgttlinger!

One thing I'm wondering -- is Functor2 (previously Bifunctor) more used than Applicative? In my experience I use the latter more, so I'd rather rename the former. But I might be wrong.

@mgttlinger
Copy link
Contributor Author

tupleA2 was there already somewhere in the context of the Applicative boilerplate so I thought that would be fitting.

@mgttlinger
Copy link
Contributor Author

Soooooo, should I change anything? Or how does this move forward?

@non
Copy link
Contributor

non commented Sep 2, 2016

@mgttlinger I think at this point we should get more sign-offs on the change, particularly since changing names is a breaking change. I'll try to ping folks in the channel to get them to weigh-in.

@mgttlinger
Copy link
Contributor Author

I will fix the conflicts once it has been decided if my changes are OK at all.

@travisbrown travisbrown mentioned this pull request Oct 24, 2016
13 tasks
@non
Copy link
Contributor

non commented Oct 25, 2016

So at this point here are the things on my mind, ranked in order of importance (from my point of view):

  1. Ensure that syntax providing methods (e.g.map2) is not ambiguous.
  2. Ensure that we don't have duplicate method names (e.g. map2) provided by syntax.
  3. Distinguish bimonad from bifunctor, bifoldable, etc.

The wrinkle here is that we've introduced a .map2 syntax on tuples to replace the applicative builder syntax (i.e. |@|). So I don't think we should switch map2 to be something on Bifunctor (or Functor2) instead, since we'd have to change this syntax as well.

In the current code, we already have (1) and (2) from my point of view (there are two ways to get .map2 syntax but they are never ambiguous: one applies to F[_] values and one applies to tuples). So the changes this PR are introducing are primarily solving (3), while hopefully not making (1) or (2) worse.

I see two clear ways forward:

First, we could rename the Bi- type classes (other than Bimonad) to numeric suffixes (Functor2, Foldable2, etc.) but leave the method names alone. This means that if we added Functor3 we'd want to add a trimap method, but that seems OK to me. If we do this there is no duplication or conflation going on, and the only slightly confusing thing is remembering that type classes with "2" suffixes have methods starting in "bi-".

The other option I see is to do nothing. I don't think the possible confusion between Bimonad and Bifunctor is currently that severe, so making major breaking changes just to fix this issue might not be worth it.

I'm fine with either of those approaches. What do you all think? @mgttlinger, what is your opinion on the best way to move forward? Please let me know if I'm missing some of the motivation for these changes.

@travisbrown
Copy link
Contributor

@non @mgttlinger Since it's not clear which direction we want to go here, mind if we push this to after 0.8.0?

@non
Copy link
Contributor

non commented Oct 25, 2016

I'm fine with putting this off until after 0.8.0 personally.

@mgttlinger
Copy link
Contributor Author

The main motivation for those changes is that a "bi-" prefix seems to communicate that something is the thing following as well as its dual in mathematics. I'm fine with delaying that change further but I think it would definitely make sense to change those names eventually. If we can come up with a cooler name than map2 for the bimap I am happy to avoid all this renaming established methods stuff. The numeric suffices aren't terribly necessary as we probably won't need much higher type-aritys (if that's the right word; kindnesses? whatever) than two or three at max.

@mgttlinger
Copy link
Contributor Author

Closing as this probably won't get merged and is too outdated by now

@mgttlinger mgttlinger closed this Sep 20, 2017
@kailuowang kailuowang modified the milestone: 1.0.0-RC1 Oct 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants