add OptionT.liftF and update related docs and tests. Close issue #659#667
Conversation
Current coverage is
|
There was a problem hiding this comment.
This may not really be something to worry about, but I'm thinking that maybe this should use Some(_) instead of Option(_). The Option.apply method has logic to turn null into None. In Cats we are generally assuming that people aren't passing around null. But if for some reason they really want to lift a null value into the OptionT structure, I don't think we should prevent them from doing that here.
There was a problem hiding this comment.
I used Option(_) to prevent null and getting None instead of Some(null) in that case, but if you confirm we should not prevent them from doing that I'll change it to Some(_).
There was a problem hiding this comment.
I think that it should be consistent with our Applicative[Option].pure implementation, which uses Some: https://github.com/non/cats/blob/master/core/src/main/scala/cats/std/option.scala#L14. So yeah, I think we should change this here. I don't think we should conflate the issues of lifting and de-nulling.
There was a problem hiding this comment.
I totally agree! I'll change it to Some(_). Thank you.
|
Thanks, @lambdista! In general, this looks great. I left a couple very minor comments. |
|
Thank you @ceedubs for your support! |
|
👍 (assuming the build passes). Thanks again! |
|
Thank you! 👍 |
|
👍 Good work! Thanks! 🐈 |
add OptionT.liftF and update related docs and tests. Close issue #659
No description provided.