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

adds more details to random.md documentation #3792

Open
wants to merge 10 commits into
base: series/3.5.x
Choose a base branch
from

Conversation

wonk1132
Copy link

Adds more detailed documentation for the Random type, including a unit test example.

@armanbilge armanbilge changed the base branch from series/3.x to series/3.5.x August 25, 2023 00:43
@armanbilge armanbilge changed the base branch from series/3.5.x to series/3.x August 25, 2023 00:43
djspiewak
djspiewak previously approved these changes Sep 2, 2023
Copy link
Member

@djspiewak djspiewak left a comment

Choose a reason for hiding this comment

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

This is great! Can we retarget at series/3.5.x though so it gets out sooner?

@armanbilge armanbilge changed the base branch from series/3.x to series/3.5.x September 2, 2023 15:54
@armanbilge armanbilge dismissed djspiewak’s stale review September 2, 2023 15:54

The base branch was changed.

docs/std/random.md Outdated Show resolved Hide resolved

// use the standard implementation of Random backed by java.util.Random()
// (the same implementation as Random.javaUtilRandom(43))
implicit val r: IO[Random[IO]] = Random.scalaUtilRandom[IO]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is having an implicit IO[A] a common pattern? I don't think I've ever seen it. I would expect an implicit Random[IO] here (but of course that opens the question of "should Random be passed implicitly").

Copy link
Author

Choose a reason for hiding this comment

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

I agree this seems odd and it doesn't make sense in this context why it's used implicitly. I changed it to be an explicit param.


// for testing, create a Random instance that gives back the same number every time. With
// this version of the Random type class, we can test our business logic works as intended.
implicit val r: IO[Random[IO]] = IO(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
implicit val r: IO[Random[IO]] = IO(
implicit val r: IO[Random[IO]] = IO.pure(

Copy link
Author

Choose a reason for hiding this comment

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

ah. yup.

implicit val r: IO[Random[IO]] = IO(
new Random[IO] {
def betweenInt(minInclusive: Int, maxExclusive: Int): IO[Int] =
IO(7) // gives back 7 every call
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
IO(7) // gives back 7 every call
IO.pure(7) // gives back 7 every call

Copy link
Author

Choose a reason for hiding this comment

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

double yup

@djspiewak
Copy link
Member

Looks like the build failures are legitimate

@wonk1132
Copy link
Author

sorry, I don't get what I need to do to fix this issue. Can someone point me the right direction?

@armanbilge
Copy link
Member

Looks like import errors. Use ._ instead of .*. Also cats.implicits is legacy, use cats.syntax.all.

Do you know how to run it locally? docs/mdoc in sbt console. Then you can see and fix the errors before you push.

Thanks for your work!

import cats.effect.std.Random
import cats.syntax.all
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import cats.syntax.all
import cats.syntax.all._

@sshark
Copy link
Contributor

sshark commented Mar 6, 2024

I feel we should retain the example of using Random in the dieRoll generic function in https://typelevel.org/cats-effect/docs/std/random#using-random

def dieRoll[F[_]: Functor: Random]: F[Int] =
  Random[F].betweenInt(0, 6).map(_ + 1) 

I would like to expand this example into a working copy-and-paste example. This example at current state does not show how dieRoll is used together with an implicit Random instance. Thank you

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.

5 participants