Utility method for encode joda time instances #276

AntonioMateoGomez · 2018-05-25T12:05:19Z

Add utility method for encoding joda.time instances.
Also adds utility for encoding from int and long to Array[Byte] in the internal module

Also adds utility for encode from int and long to Array[Byte].

fedefernandez

Please, remove the byte array related methods from JavaTimeUtil and update the encoders and the tests

fedefernandez · 2018-05-25T12:29:47Z

modules/internal/src/test/scala/util/EncoderUtilTest.scala

+          val value: Array[Byte] = EncoderUtil.longToByteArray(n)
+
+          EncoderUtil.byteArrayToLong(value) shouldBe n
+          EncoderUtil.byteArrayToLong(value) == n


This was a mistake in my first PR, you could remove the shouldBe statement

fedefernandez · 2018-05-25T12:30:43Z

modules/marshallers/jodatime/src/test/scala/util/JodaTimeUtilTest.scala

+          val date: LocalDateTime = dt.toDateTime(DateTimeZone.UTC).toLocalDateTime
+          val value: Long         = JodaTimeUtil.JodaLocalDatetimeToLong(date)
+
+          JodaTimeUtil.longToJodaLocalDateTime(value) shouldBe date


fedefernandez · 2018-05-25T12:30:48Z

modules/marshallers/jodatime/src/test/scala/util/JodaTimeUtilTest.scala

+          val date: LocalDate = dt.toLocalDate
+          val value: Int      = JodaTimeUtil.jodaLocalDateToInt(date)
+
+          JodaTimeUtil.intToJodaLocalDate(value) shouldBe date


Remove this line, please

fedefernandez · 2018-05-25T12:30:58Z

modules/internal/src/test/scala/util/EncoderUtilTest.scala

+        { n: Int =>
+          val value: Array[Byte] = EncoderUtil.intToByteArray(n)
+
+          EncoderUtil.byteArrayToInt(value) shouldBe n


Please, remove this line

fedefernandez · 2018-05-25T12:31:55Z

modules/marshallers/jodatime/src/main/scala/util/JodaTimeUtil.scala

+import org.joda.time._
+
+object JodaTimeUtil {
+


Could you please add a val here with the initial date? Something like:

private[this] val initialDate = DateTime.now(DateTimeZone.UTC).withMillis(0)

and updated the encoders and the tests. Also removed the shouldBe statement from JodaTimeUtilTest and EncoderUtilTest

fedefernandez · 2018-05-25T21:03:36Z

modules/internal/src/test/scala/util/JavaTimeUtilTests.scala

@@ -49,8 +49,9 @@ class JavaTimeUtilTests extends WordSpec with Matchers with Checkers {
      check {
        forAll(genDateTimeWithinRange(from, range)) { zdt: ZonedDateTime =>
          val date  = zdt.toLocalDate
-          val value = JavaTimeUtil.localDateToByteArray(date)
-          JavaTimeUtil.byteArrayToLocalDate(value) == date


Please, remove this test. We've removed this method.

fedefernandez · 2018-05-25T21:04:01Z

modules/internal/src/test/scala/util/JavaTimeUtilTests.scala

@@ -71,8 +72,8 @@ class JavaTimeUtilTests extends WordSpec with Matchers with Checkers {
      check {
        forAll(genDateTimeWithinRange(from, range)) { zdt: ZonedDateTime =>
          val date  = zdt.toLocalDateTime
-          val value = JavaTimeUtil.localDateTimeToByteArray(date)
-          JavaTimeUtil.byteArrayToLocalDateTime(value) == date


Same as above, please remove this test

fedefernandez · 2018-05-25T21:04:44Z

modules/marshallers/jodatime/src/main/scala/util/JodaTimeUtil.scala

+  def jodaLocalDateToInt(value: LocalDate): Int =
+    Days
+      .daysBetween(
+        DateTime.now(DateTimeZone.UTC).withMillis(0),


Replace this by the initalDate val

fedefernandez · 2018-05-25T21:04:59Z

modules/marshallers/jodatime/src/main/scala/util/JodaTimeUtil.scala

+      .getDays
+
+  def intToJodaLocalDate(value: Int): LocalDate =
+    DateTime.now().withMillis(0).plusDays(value).toLocalDate


Same as above

Use of the initialDate val in the JodaTimeUtil methods.

L-Lavigne

Thanks for this PR! I have some comments about modules and dependencies, please take a look.

L-Lavigne · 2018-05-25T22:08:47Z

build.sbt

+//// MARSHALLERS ////
+/////////////////////
+
+lazy val `marshallers-jodatime` = project


Should this really be in its own module? Makes sense if our practice is to do so when introducing new dependencies, otherwise I suggest integrating in an existing module.

In any case if making a new module, we should add it to the list in quickstart.md.

IMO it's a good idea to put it in it's own module in order to avoid adding more transitive dependencies. And yes, good catch, we should add it to the quickstart

Sounds good. We could also add a sentence or two in the docs about the new marshallers.

I'd add the information in the docs once we solved #269 because now it's not ready for its usage. @L-Lavigne @juanpedromoreno thoughts?

L-Lavigne · 2018-05-25T22:09:42Z

build.sbt

+
+lazy val `marshallers-jodatime` = project
+  .in(file("modules/marshallers/jodatime"))
+  .dependsOn(common % "compile->compile;test->test")


Do we need all those dependencies? I don't see them imported in the code. (Possibly more dependencies are needed for unit tests, but those should be only in test scopes and not exported).

I think they will be used in the next PR for adding the marshallers but we could remove them for now 👍

Removed unused dependencies from the marshallers module in the `build.sbt`. Minor changes in `JodaTimeUtil`

L-Lavigne · 2018-05-29T00:17:30Z

build.sbt

+//// MARSHALLERS ////
+/////////////////////
+
+lazy val `marshallers-jodatime` = project


Sounds good. We could also add a sentence or two in the docs about the new marshallers.

L-Lavigne · 2018-05-29T00:18:00Z

docs/src/main/tut/quickstart.md

@@ -28,6 +28,7 @@ It's divided into multiple and different artifacts, grouped by scope:
 `frees-rpc-client-netty` | Client | Yes* | Mandatory on the client side if we are using `Netty` in the server side.
 `frees-rpc-client-okhttp` | Client | Yes* | Mandatory on the client side if we are using `OkHttp` in the server side.
 `frees-rpc-config` | Server/Client | No | Provides configuration helpers using [frees-config] to load the application configuration values.
+`frees-rpc-marshallers` | Server/Client | No | Provides marshallers for serializing and deserializing the `LocalDate` and `LocalDateTime` joda instances.


frees-rpc-marshallers-jodatime

fedefernandez

Looks good, thanks.

juanpedromoreno · 2018-07-09T21:56:45Z

This is no longer blocked or waiting for the 0.14.0 ;)

…tilities

juanpedromoreno

👏

fedefernandez · 2018-07-10T12:51:14Z

modules/internal/src/test/scala/util/EncoderUtilTest.scala

+
+    "allow to convert int to and from byteArray" in {
+      check {
+        { n: Int =>


here is missing the forAll right? maybe I'm missing something but I think this should be:

check { forAll { n: Int => val value: Array[Byte] = EncoderUtil.intToByteArray(n) EncoderUtil.byteArrayToInt(value) == n } }

Yes Fede, you are right.

Ok, we can tackle it in the next PR. Thanks

Add utility method for encode joda.time instances.

72b7f38

Also adds utility for encode from int and long to Array[Byte].

AntonioMateoGomez requested a review from fedefernandez May 25, 2018 12:05

fedefernandez reviewed May 25, 2018

View reviewed changes

Removed Array[Byte] related methods from JavaTimeUtil,

e54771e

and updated the encoders and the tests. Also removed the shouldBe statement from JodaTimeUtilTest and EncoderUtilTest

fedefernandez reviewed May 25, 2018

View reviewed changes

Removed Array[Byte] related test from JavaTimeUtilTest.

3f93f8c

Use of the initialDate val in the JodaTimeUtil methods.

L-Lavigne reviewed May 25, 2018

View reviewed changes

Included frees-rpc-marshallers module in quickstart.md

df44fb2

Removed unused dependencies from the marshallers module in the `build.sbt`. Minor changes in `JodaTimeUtil`

L-Lavigne reviewed May 29, 2018

View reviewed changes

Artifact name renamed in the quickstart.md

d2f94c5

fedefernandez approved these changes May 29, 2018

View reviewed changes

Merge remote-tracking branch 'origin/master' into feature/joda-time-u…

d299384

…tilities

juanpedromoreno approved these changes Jul 10, 2018

View reviewed changes

AntonioMateoGomez merged commit ed9c222 into master Jul 10, 2018

fedefernandez reviewed Jul 10, 2018

View reviewed changes

diesalbla deleted the feature/joda-time-utilities branch May 7, 2019 15:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Utility method for encode joda time instances #276

Utility method for encode joda time instances #276

AntonioMateoGomez commented May 25, 2018

fedefernandez left a comment

fedefernandez May 25, 2018

fedefernandez May 25, 2018

fedefernandez May 25, 2018

fedefernandez May 25, 2018

fedefernandez May 25, 2018

fedefernandez May 25, 2018

fedefernandez May 25, 2018

fedefernandez May 25, 2018

fedefernandez May 25, 2018

L-Lavigne left a comment

L-Lavigne May 25, 2018 •

edited

Loading

L-Lavigne May 25, 2018

fedefernandez May 26, 2018 •

edited

Loading

L-Lavigne May 29, 2018

fedefernandez May 29, 2018

L-Lavigne May 25, 2018

fedefernandez May 26, 2018

L-Lavigne May 29, 2018

L-Lavigne May 29, 2018

fedefernandez left a comment

juanpedromoreno commented Jul 9, 2018

juanpedromoreno left a comment

fedefernandez Jul 10, 2018

AntonioMateoGomez Jul 10, 2018

fedefernandez Jul 10, 2018

		import org.joda.time._

		object JodaTimeUtil {

Utility method for encode joda time instances #276

Utility method for encode joda time instances #276

Conversation

AntonioMateoGomez commented May 25, 2018

fedefernandez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

L-Lavigne left a comment

Choose a reason for hiding this comment

L-Lavigne May 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedefernandez May 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedefernandez left a comment

Choose a reason for hiding this comment

juanpedromoreno commented Jul 9, 2018

juanpedromoreno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

L-Lavigne May 25, 2018 •

edited

Loading

fedefernandez May 26, 2018 •

edited

Loading