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

Added examples for Cokleisli #1993

Merged
merged 26 commits into from
Oct 29, 2017
Merged

Added examples for Cokleisli #1993

merged 26 commits into from
Oct 29, 2017

Conversation

raymondtay
Copy link
Contributor

Thought it might be helpful to include examples on dimap, lmap and rmap for learners of the library

@codecov-io
Copy link

codecov-io commented Oct 25, 2017

Codecov Report

Merging #1993 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1993   +/-   ##
=======================================
  Coverage   95.49%   95.49%           
=======================================
  Files         298      298           
  Lines        4814     4814           
  Branches      120      120           
=======================================
  Hits         4597     4597           
  Misses        217      217
Impacted Files Coverage Δ
core/src/main/scala/cats/data/Cokleisli.scala 100% <ø> (ø) ⬆️

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 58a9fd6...3242791. Read the comment docs.

* scala> def before(x: Int) = x + 1
* scala> def after(x: Int) = x - 1
* scala> val example : Cokleisli[Id,Int,Int] = Cokleisli((f: Id[Int]) => f.extract)
* scala> example.dimap(before)(after) == 42
Copy link
Member

Choose a reason for hiding this comment

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

Can you add what this returns here and below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @LukaJCB for the reminder, appreciate it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johnynek thanks for the review and agree with you. Following on your lead, i've revised the examples to use NonEmptyList instead, let me know what you think when you look at them?

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.

I appreciate you sending the PR. Could we find an example with M not being Id? The problem with Id is that it makes many typeclasses trivial and does not really give much of an idea what is going on.

What about M = NonEmptyList?

@raymondtay
Copy link
Contributor Author

@johnynek thanks for the review and i agree with you. Following your suggestion, i've changed it examples to using NonEmptyList instead of Id. Let me know what you think of those changes?

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

Thanks! I like these NEL examples. One minor suggestion below.

* scala> example.dimap(before)(after) == 42
* scala> import cats._, data._
* scala> val f = Cokleisli((xs: NonEmptyList[Int]) => xs.reverse.head)
* f: cats.data.Cokleisli[cats.data.NonEmptyList,Int,Int] = Cokleisli(<function1>)
Copy link
Member

Choose a reason for hiding this comment

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

I'd omit these toString renderings, just like you omitted the output of the defs. The other examples in Cats tend to omit the trivial outputs and focus on the setup and the res0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rossabaker gotcha :)

rossabaker
rossabaker previously approved these changes Oct 28, 2017
Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

👍

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.

One minor comment. I hope you’ll indulge me but if you don’t have time we can merge and deal with it later.

* {{{
* scala> import cats._, data._
* scala> val f = Cokleisli((xs: NonEmptyList[Int]) => xs.reverse.head)
* scala> def before(x: String) = x.toInt
Copy link
Contributor

Choose a reason for hiding this comment

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

I hate to be a jerk, but can we use a function that won’t throw. I know this example is safe but using toInt in general is dangerous and I hate to set the example in docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree

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.

Thanks!

@kailuowang kailuowang merged commit c4ee747 into typelevel:master Oct 29, 2017
@kailuowang kailuowang added this to the 1.0.0-RC1 milestone Oct 30, 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.

7 participants