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

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

3 participants