Cleanup storehaus-mongodb #324
Conversation
|
ping @johnynek in case you missed it |
| } | ||
| case None => | ||
| if (valueObject.get(valueName).isEmpty) None | ||
| else throw new ClassCastException("Cannot convert %s to %s" |
There was a problem hiding this comment.
can we use s"Cannot convert ${value.getClass} to ${manifest[V]}"
|
one minor comment (about string formatting). I would say that maybe this code needs to be reworked. It might make sense to move it to reactivemongo http://reactivemongo.org/ which is already async and handles most of the issues here (but that is out of scope for the current PR). |
|
Agreed, will tackle string formatting here and create an issue to move to reactive mongo. |
| } | ||
| } | ||
| } | ||
|
|
||
| protected def getValue(valueObject: MongoDBObject): Option[V] = { | ||
| protected def getValue(valueObject: MongoDBObject): Try[V] = |
There was a problem hiding this comment.
I think we want Try[Option[V]] here actually. Some things are legitimately None.
There was a problem hiding this comment.
Yeah, that's what I wanted your input on earlier.
There was a problem hiding this comment.
Sorry I overlooked that before.
On Wed, Aug 24, 2016 at 19:51 Ben Fradet [email protected] wrote:
In
storehaus-mongodb/src/main/scala/com/twitter/storehaus/mongodb/MongoStore.scala
#324 (comment):} }}
- protected def getValue(valueObject: MongoDBObject): Option[V] = {
- protected def getValue(valueObject: MongoDBObject): Try[V] =
Yeah, that's what I wanted your input on earlier.
—
You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub
https://github.com/twitter/storehaus/pull/324/files/ec02fb3c5fc5b7878aa97b80d3f6873fedd2e263#r76183627,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEJdhQgTrOLaYVQoWj6ngIrSaNk8Ee8ks5qjS1TgaJpZM4Jefzw
.
|
Thanks a lot for your review, sorry if I wasn't clear enough earlier. |
|
👍 |
No description provided.