-
-
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
Fix Free Monad example #4253
Fix Free Monad example #4253
Conversation
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!
State.inspect(_.get(key).map(_.asInstanceOf[A])) | ||
State.inspect(_.get(key).asInstanceOf[A]) |
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.
Hmm, I may have merged this a bit too hastily 😓
.get
returns an Option
, so we can't cast to it to A
. Seems like what we should be doing here is directly calling .apply
on the key
.
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.
Also, it's annoying that mdoc in CI passes for this even if it's not working.
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 review my pull request.
The definition of Get
is case class Get[T](key: String) extends KVStoreA[Option[T]]
.
In this case, I think type A
is Option[T]
🤔.
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.
@youta32449999 ah, indeed your are right.,I am confusing A
with T
. Hasty reviewing again! 😅 Sorry about that, and thanks for clarifying. I guess the reason it worked before is because of type erasure at runtime, so that the mistake did not matter.
It looks to be correct on the website. Thank you again for fixing this!
https://typelevel.org/cats/datatypes/freemonad.html#_5-run-your-program
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 I am also confused by the comment.
cats/docs/datatypes/freemonad.md
Line 172 in 038705e
// the program will crash if a key is not found, |
Since Get
returns an Option
, in what situation will it crash?
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.
Hmm, I think it will crash if a type is incorrectly specified.
For example:
def program: KVStore[Option[Int]] = for {
_ <- put("wild-cats", "two") // expect Int, but value is String.
_ <- update[Int]("wild-cats", (_ + 12))
_ <- put("tame-cats", 5)
n <- get[Int]("wild-cats")
_ <- delete("tame-cats")
} yield n
So, I'm going to fix the comment as below.
the program will crash if a type is incorrectly specified.
Type Mismatch Error
occurredimpureCompiler
andpureCompiler
in scalaVersion 3.1.3