-
Notifications
You must be signed in to change notification settings - Fork 125
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
Add a module for finagle bijections. #197
Conversation
@johnynek @folone is this the right place to put this? I was thinking that it would be OK to just make finagle-mysql take a dependency on bijections, but I don't have really strong opinions about this. Although it might make more sense to make this a |
limitations under the License. | ||
*/ | ||
|
||
package com.twitter.bijection.twitter_finagle |
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.
twitter_finagle? are there other finagle projects? should drop the twitter_
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.
+1, also note the com.twitter prefix.
This looks great, aside from a few minor suggestions. Thanks so much for doing this! |
Addressed the review in 55b7e8b. |
LGTM |
private val UTC = java.util.TimeZone.getTimeZone("UTC") | ||
private val timestampValue = new TimestampValue(UTC, UTC) | ||
def apply(t: Timestamp) = timestampValue(t) | ||
override def invert(v: Value) = Inversion.attempt(timestampValue.unapply(v).get)(identity) |
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.
this is not how attempt works. Yours will throw if the inversion fails. It should be:
Inversion.attempt(v) { v => timestampValue.unapply(v).get }
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.
Oh! Thanks, fixed!
one minor issue. |
55b7e8b
to
547585a
Compare
Thanks! |
Add a module for finagle bijections.
Thanks! |
@folone your tests are failing. Instead of extending
|
@MansurAshraf I believe @johnynek already merged this PR though. Should I send another one against your branch? |
@folone yes thats fine. Thank you! |
@MansurAshraf I just tried extending Compiling garden.css...
diff --git a/bijection-finagle-mysql/src/test/scala/com/twitter/bijection/finagle_mysql/MySqlConversionLaws.scala b/bijection-finagle-mysql/src/test/scala/com/twitter/bijection/finagle_mysql/MySqlConversionLaws.scala
index 7328abd..ffc64f7 100644
--- a/bijection-finagle-mysql/src/test/scala/com/twitter/bijection/finagle_mysql/MySqlConversionLaws.scala
+++ b/bijection-finagle-mysql/src/test/scala/com/twitter/bijection/finagle_mysql/MySqlConversionLaws.scala
@@ -18,8 +18,6 @@ package com.twitter.bijection.finagle_mysql
import com.twitter.bijection.{ CheckProperties, BaseProperties, Bijection }
import org.scalacheck.Arbitrary
-import org.scalatest.{ PropSpec, MustMatchers }
-import org.scalatest.prop.PropertyChecks
import com.twitter.finagle.exp.mysql._
import java.sql.Timestamp
import java.util.Date
@@ -29,7 +27,7 @@ import org.scalacheck.Gen
import org.scalacheck.Arbitrary
import org.scalacheck.Arbitrary.arbitrary
-class MySqlConversionLaws extends PropSpec with PropertyChecks with MustMatchers with BaseProperties {
+class MySqlConversionLaws extends CheckProperties with BaseProperties {
import MySqlConversions._
implicit val byteValueArb = Arbitrary(arbitrary[Byte].map(ByteValue.apply)) |
@MansurAshraf I'll have some time to look at it on Friday. I'm wondering how those tests were working before #198 though. |
@folone they were not running. See twitter/algebird#421 (its for algebird but same issue was happening in this project) |
@MansurAshraf is right. I missed that in the review. We had a bug in some our tests after the migration to scalatest, and your code copied that pattern. |
Here's the fix: #201 |
As suggested in twitter/finagle#341, implemented
Bijection
s andInjection
s for finagle-mysql datatypes./cc @mosesn @fwbrasil @williamboxhall