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

Functor.fmap #2056

Merged
merged 4 commits into from
Dec 1, 2017
Merged

Functor.fmap #2056

merged 4 commits into from
Dec 1, 2017

Conversation

fosskers
Copy link
Contributor

@fosskers fosskers commented Dec 1, 2017

This PR adds a .fmap method to Functor as an alias of .map. This is in the spirit of Foldable.combineAll which is an alias for .fold, as many types have a built-in .fold with different behaviour, preventing .fold from being injected.

PROs

  • .fmap is injectable, exposing lawful Functorness to implementing classes (i.e. Map has a built-in .map that breaks Functor laws, but its injected .fmap would be lawful). This also benefits authors of new Functor instances.
  • Haskellers get a warm, fuzzy feeling of familiarity.

CONs

  • Increased API surface area for a method that is not strictly necessary (but then you'd have to say the same for .combineAll)
  • Potential confusion with .flatMap for non-Haskellers.

Copy link
Contributor

@johnynek johnynek left a comment

Choose a reason for hiding this comment

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

If we add this it should be final. But could we just add it as syntax and not to the typeclass?

@fosskers
Copy link
Contributor Author

fosskers commented Dec 1, 2017

final so that it can't be overridden, yeah?

@fosskers
Copy link
Contributor Author

fosskers commented Dec 1, 2017

Foldable.combineAll is included directly in the typeclass too.

@SystemFw
Copy link
Contributor

SystemFw commented Dec 1, 2017

I don't have any strong opinions on this, and avoid having to do Functor[Blah].map seems useful enough, however wrt "haskellers get a warm-fuzzy feeling of familiarity", this is a case where actually (as a Haskeller), things got fixed here.
Haskell used to have fmap be called just map. Then a map specialised to Lists was added in the early days to avoid complicating things for beginners that don't know about typeclasses. However, after the FTP proposal has brought polymorphism back into the Prelude, Functor fmap and a special map for List stick out like a sore thumb.

@LukaJCB
Copy link
Member

LukaJCB commented Dec 1, 2017

Hi @fosskers, can you make a mima exception, please? :)

@kailuowang kailuowang added this to the 1.0.0 milestone Dec 1, 2017
@kailuowang kailuowang mentioned this pull request Dec 1, 2017
3 tasks
@fosskers
Copy link
Contributor Author

fosskers commented Dec 1, 2017

@SystemFw The fmap / map situation in Haskell is weird, yes. Hopefully we can settle on something nice there too. For Cats, it's a problem of "Well .map can't be injected. Okay let's make an alias. What to call it? Okay, .fmap because of prescedent and intuition: fmap = Functor map."

@codecov-io
Copy link

codecov-io commented Dec 1, 2017

Codecov Report

Merging #2056 into master will decrease coverage by 0.3%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2056      +/-   ##
==========================================
- Coverage   94.95%   94.64%   -0.31%     
==========================================
  Files         311      318       +7     
  Lines        5270     5364      +94     
  Branches      133      119      -14     
==========================================
+ Hits         5004     5077      +73     
- Misses        266      287      +21
Impacted Files Coverage Δ
core/src/main/scala/cats/Functor.scala 100% <100%> (ø) ⬆️
core/src/main/scala/cats/instances/eq.scala 80% <0%> (-20%) ⬇️
core/src/main/scala/cats/instances/order.scala 83.33% <0%> (-16.67%) ⬇️
core/src/main/scala/cats/data/IndexedStateT.scala 89.36% <0%> (-9.45%) ⬇️
core/src/main/scala/cats/instances/function.scala 92.3% <0%> (-7.7%) ⬇️
core/src/main/scala/cats/data/OptionT.scala 97.46% <0%> (-2.54%) ⬇️
core/src/main/scala/cats/data/WriterT.scala 93.13% <0%> (-0.62%) ⬇️
core/src/main/scala/cats/syntax/either.scala 99.12% <0%> (-0.02%) ⬇️
core/src/main/scala/cats/instances/ordering.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/data/Ior.scala 100% <0%> (ø) ⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40465ac...d7d347d. Read the comment docs.

@SystemFw
Copy link
Contributor

SystemFw commented Dec 1, 2017

Sure, I agree that the basic problem of underlying methods taking precedence over typeclass ones is there.
I guess the question is how many instances of this are out there for the map case? And the other question is how big of a problem it actually is. Unless people always call fmap , you still need to know whether the map you're calling is a good one, or a bad one, in which case you can do Functor[F].map.

In any case this is making me sound way more opposed to the proposal than I actually am. My only concern is that an advanced user knows that Functor[F].map will work, and a beginner might be confused as to why fmap is similar to flatMap, especially given that the name fmap is not nice in Haskell either. Unfortunately I can't think of a better name that isn't map or lift, both already taken.

LukaJCB
LukaJCB previously approved these changes Dec 1, 2017
Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

This is a great addition, thank you very much @fosskers!

@fosskers
Copy link
Contributor Author

fosskers commented Dec 1, 2017

Lol wuh github? I didn't dismiss a review.

@LukaJCB
Copy link
Member

LukaJCB commented Dec 1, 2017

It automatically dismissed reviews when you push, don't worry :)

Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

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

thanks!

@LukaJCB LukaJCB merged commit 6e52dfd into typelevel:master Dec 1, 2017
@fosskers fosskers deleted the functor-fmap branch December 1, 2017 20:03
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.

6 participants