-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add more DocTests for OptionT
#4311
Conversation
* scala> import cats.data.OptionT | ||
* scala> import scala.util.Try | ||
* | ||
* scala> // prints "5" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a good way to test if the side-effects worked properly?
If not, should we even leave those examples in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, and Try
is not sufficient for suspending side-effects anyway (nothing in Cats is). We should change these examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, OptionT
does not have to deal with side effects:
scala> import cats.data.OptionT, cats.syntax.all._
import cats.data.OptionT
import cats.syntax.all._
scala> val optt = OptionT.some[Either[String, *]](3)
val optt = OptionT(Right(Some(3)))
scala> optt.semiflatTap { case 1 | 2 | 3 => Right("hit!"); case _ => Left("miss!") }
val res0 = OptionT(Right(Some(3)))
scala> optt.semiflatTap { case 0 | 1 | 2 => Right("hit!"); case _ => Left("miss!") }
val res1 = OptionT(Left(miss!))
UPD. I stripped out information on types for better readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's very clever, I added it to the PR! 👍
OptionT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for this work. Overall looks great, just a few comments. Sorry for the slow review cycle.
I also kicked off CI, I didn't realize it was pending approval.
* | ||
* Example: | ||
* {{{ | ||
* scala> import cats.implicits._ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cats.implicits
is deprecated.
* scala> import cats.implicits._ | |
* scala> import cats.syntax.all._ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that was news to me. There were actually a bunch of unused cats.implicits._
imports in the Doctests.
I removed them.
* scala> import cats.implicits._ | ||
* scala> import cats.data.OptionT | ||
* | ||
* scala> type ToString[A] = Function1[A, String] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is cats.Show
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course 🤦 - I was so happy I finally found an example for this I didn't even notice.
Updated.
* scala> import cats.data.OptionT | ||
* scala> import scala.util.Try | ||
* | ||
* scala> // prints "5" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, and Try
is not sufficient for suspending side-effects anyway (nothing in Cats is). We should change these examples.
* scala> import cats.data.OptionT | ||
* | ||
* scala> val optionT: OptionT[List, Int] = OptionT[List, Int](List(Some(2), Some(3), Some(4))) | ||
* scala> optionT.mapFilter(x => Option.when(x % 2 == 0)(x)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI might not like Option.when
on Scala 2.12. I guess we'll find out :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, I changed to Option(...).filter(...)
.
I assume with ~+coreJVM/testOnly **OptionTDoctest
(added the +
) I should catch those problems, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly. I don't know how ~
and +
interact in sbt.
* scala> val optionT: OptionT[Try, Int] = OptionT[Try, Int](Try(None)) | ||
* scala> optionT.flatTapNone(Try(println("no value"))).value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this example is also not really good for the same reason as one for semiflatTap
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I applied the same idea as in semiflatTap
here too. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this substantial contribution!!
#2479