Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add more DocTests for OptionT #4311

Merged
merged 3 commits into from
Nov 12, 2022
Merged

Conversation

timo-schmid
Copy link
Contributor

* scala> import cats.data.OptionT
* scala> import scala.util.Try
*
* scala> // prints "5"
Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor

@satorg satorg Oct 12, 2022

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.

Copy link
Contributor Author

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! 👍

@armanbilge armanbilge changed the title Add more DocTests for OptionT Add more DocTests for OptionT Oct 2, 2022
@armanbilge armanbilge added this to the 2.9.0 milestone Oct 2, 2022
Copy link
Member

@armanbilge armanbilge left a 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._
Copy link
Member

Choose a reason for hiding this comment

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

cats.implicits is deprecated.

Suggested change
* scala> import cats.implicits._
* scala> import cats.syntax.all._

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, 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]
Copy link
Member

Choose a reason for hiding this comment

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

This is cats.Show :)

Copy link
Contributor Author

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"
Copy link
Member

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))
Copy link
Member

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 :)

Copy link
Contributor Author

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?

Copy link
Member

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.

Comment on lines 283 to 284
* scala> val optionT: OptionT[Try, Int] = OptionT[Try, Int](Try(None))
* scala> optionT.flatTapNone(Try(println("no value"))).value
Copy link
Contributor

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.

Copy link
Contributor Author

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. 👍

Copy link
Member

@armanbilge armanbilge left a 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!!

@armanbilge armanbilge merged commit 85d0355 into typelevel:main Nov 12, 2022
@timo-schmid timo-schmid deleted the doctests-optiont branch November 25, 2022 10:19
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants