Skip to content

Fix Free Monad example #4253

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.

Sign up for GitHub

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

Merged
armanbilge merged 1 commit into from
Jun 25, 2022
Merged

Fix Free Monad example #4253

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/datatypes/freemonad.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ def impureCompiler: KVStoreA ~> Id =
()
case Get(key) =>
println(s"get($key)")
kvs.get(key).map(_.asInstanceOf[A])
kvs.get(key).asInstanceOf[A]
case Delete(key) =>
println(s"delete($key)")
kvs.remove(key)
Expand Down Expand Up @@ -273,7 +273,7 @@ val pureCompiler: KVStoreA ~> KVStoreState = new (KVStoreA ~> KVStoreState) {
fa match {
case Put(key, value) => State.modify(_.updated(key, value))
case Get(key) =>
State.inspect(_.get(key).map(_.asInstanceOf[A]))
State.inspect(_.get(key).asInstanceOf[A])
Copy link
Copy Markdown
Member

@armanbilge armanbilge Jun 25, 2022

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.

Copy link
Copy Markdown
Member

@armanbilge armanbilge Jun 25, 2022

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.

Copy link
Copy Markdown
Contributor Author

@youta32449999 youta32449999 Jun 26, 2022

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

Copy link
Copy Markdown
Member

@armanbilge armanbilge Jun 26, 2022

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

Copy link
Copy Markdown
Member

@armanbilge armanbilge Jun 26, 2022

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?

Copy link
Copy Markdown
Contributor Author

@youta32449999 youta32449999 Jun 26, 2022

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.

case Delete(key) => State.modify(_ - key)
}
}
Expand Down