Documentation refresh for monad transformers #4724
Conversation
There was a problem hiding this comment.
NICE!!! 🎉
Thanks so much for putting in this effort, I'm really excited about it.
I think this is a good organizational change, I like the reduced Future usage, and less transformers in the return types.
I think we can go a little bit further calling out the pitfalls. Namely, let's repeat them on their respective transformer pages.
And let's call out the return types advice more prominently.
| @@ -3,7 +3,7 @@ | |||
| API Documentation: @:api(cats.data.EitherT) | |||
|
|
|||
| `Either` can be used for error handling in most situations. However, when | |||
| `Either` is placed into effectful types such as `Option` or`Future`, a large | |||
| `Either` is placed into effectful types such as `Option`, a large | |||
There was a problem hiding this comment.
There's still an example using Future below this, but I think if we change that it should happen in another PR.
There was a problem hiding this comment.
Ya. Examples are awkward since one of the big motivations is IO, but we can't depend on CE here.
docs/monadtransformers/index.md
Outdated
| The above `map` will return a value of type `OptionT[Try, String]`. | ||
|
|
||
| To get the underlying `Try[Option[String]]` value, simply call `.value` on the `OptionT` instance. | ||
| In practice, you should not include monad transformers in function signatures, and only use them as locally-scoped utilities to reduce boilerplate so that calling code is not required to care about the composition. |
There was a problem hiding this comment.
This sentence is kind of buried here, but I think it was a key point of the recent discussion about transformers.
Can we perhaps highlight this in a section of its own?
| class Account { /* ... */ } | ||
| class Money { /* ... */ } | ||
|
|
||
| def findUserById(userId: Long): Try[Option[User]] = { /* ... */ ??? } |
There was a problem hiding this comment.
Just want to note that these have changed from
OptionT[Future, User] to Try[Option[User]]
I think this is a good improvement if we're trying to recommend that folks don't use transformers in return types.
docs/monadtransformers/index.md
Outdated
| While powerful, this can lead to unwieldy type signatures and poor type inference. | ||
| The [Cats MTL](https://typelevel.org/cats-mtl/) project provides a capability based approach to extend effect types without requiring as much boilerplate. | ||
|
|
||
| Some monad transformers can also interact poorly with more powerful effect types that provide concurrent computation, such as Cats Effect's `IO` or fs2's `Stream`. You should avoid `StateT` and `WriterT` and use the concurrent data types provided by those libraries instead. Because `IO` already provides its own error channel, `EitherT[IO, Throwable]` can also lead to confusing behavior; prefer `IO`'s own error channel instead. |
There was a problem hiding this comment.
I think this warning should definitely be repeated on the EitherT, StateT, and WriterT, pages.
There was a problem hiding this comment.
Thanks for the screenshots.
I think these are good.
The warning on EitherT is a little too up top front and center, but I don't think that's a blocker.
There was a problem hiding this comment.
I couldn't identify a nice place to put that one, EitherT doesn't have a convenient intro like the other ones.
reardonj
force-pushed
the
documentation-refresh-monad-transformers
branch
from
518bcd6 to
9264f63
Compare
- add warnings to specific transformer pages - call out recommended usage more specifically - add CE link
This PR came out of a discussion in the Typelevel discord with @djspiewak and @armanbilge regarding unexpected interactions with monad transformers and Cats Effect.
This PR moves around monad transformer documentation to better reflect that it is a more advanced topic:
FuturetoTryto avoid suggesting developers use the deprecated Future instancesI also made a few related changes for consistency: