Skip to content

Functor.fmap - #2056

Merged
LukaJCB merged 4 commits into
typelevel:masterfrom
fosskers:functor-fmap
Dec 1, 2017
Merged

Functor.fmap#2056
LukaJCB merged 4 commits into
typelevel:masterfrom
fosskers:functor-fmap

Conversation

@fosskers

@fosskers fosskers commented Dec 1, 2017

Copy link
Copy Markdown
Contributor

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.

@johnynek johnynek left a comment

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.

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

@fosskers

fosskers commented Dec 1, 2017

Copy link
Copy Markdown
Contributor Author

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

@fosskers

fosskers commented Dec 1, 2017

Copy link
Copy Markdown
Contributor Author

Foldable.combineAll is included directly in the typeclass too.

@SystemFw

SystemFw commented Dec 1, 2017

Copy link
Copy Markdown
Contributor

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

LukaJCB commented Dec 1, 2017

Copy link
Copy Markdown
Member

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

fosskers commented Dec 1, 2017

Copy link
Copy Markdown
Contributor Author

@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

codecov-io commented Dec 1, 2017

Copy link
Copy Markdown

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

SystemFw commented Dec 1, 2017

Copy link
Copy Markdown
Contributor

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

@LukaJCB LukaJCB left a comment

Copy link
Copy Markdown
Member

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

fosskers commented Dec 1, 2017

Copy link
Copy Markdown
Contributor Author

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

@LukaJCB

LukaJCB commented Dec 1, 2017

Copy link
Copy Markdown
Member

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

@kailuowang kailuowang left a comment

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.

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