Implement Instant.parse and LocalDate.parse. #36

chrisshafer · 2017-11-16T02:04:41Z

Implemented parse method for java.time.Instant
Implemented parse method for java.time.LocalDate
Fixed bug with toString method in java.time.Instant

Fixes: #35

chrisshafer · 2017-11-16T02:06:27Z

Hi! I wasn't quite sure what path to take when implementing the parsing functionality. Please let me know if this is not the right approach.

sjrd · 2017-11-16T09:44:56Z

Thank you for your contribution.

Before I take a look at your PR, could you please confirm that you did not look at the source code of java.time of OpenJDK nor OracleJDK? See https://github.com/scala-js/scala-js-java-time/blob/master/CONTRIBUTING.md

chrisshafer · 2017-11-16T16:06:38Z

Of Course!
I did not look at the source code of java.time of either OpenJDK or OracleJDK.

chrisshafer · 2017-12-16T21:12:01Z

@sjrd Would it be possible to get a review please 😄 ?

sjrd · 2017-12-17T09:03:42Z

Ah sorry, I forgot about this. I'll try to fit it sometime this week.

chrisshafer · 2018-01-15T04:08:29Z

Sorry, this got closed accidentally by a bad reference in another project.

sjrd

Thanks. I have finally found some time for this.

I have a few comments, mostly coding style things. I have only reported each kind of coding style issue once, so make sure to look for other places in your PR where the same comments apply.

sjrd · 2018-01-16T15:10:29Z

src/main/scala/java/time/Instant.scala

+      case years if years > 9999 => s"+$years"
+      case years if years < 0 && years > -1000 =>
+        "-%04d".format(Math.abs(years))
+      case years => years.toString


We typically do not use match constructs for what would otherwise be a chain an if/else chain. The style would be more like:

val years = hi * 10000 + date.getYear val yearSegment = { if (years > 9999) s"+$years" else if (years < 0 && years > -1000) "-%04d".format(-years) else years.toString }

(also, Math.abs(years) is advantageously replaced by -years since we already it is < 0 at that point)

sjrd · 2018-01-16T15:10:59Z

src/main/scala/java/time/Instant.scala

    }

+    val monthSegement = "%02d".format(date.getMonthValue)
+    val daySegment    = "%02d".format(date.getDayOfMonth)


In our coding style, we do not use vertical alignment (except for arrows of single-line cases).

sjrd · 2018-01-16T15:13:38Z

src/main/scala/java/time/Instant.scala

  final val EPOCH = new Instant(0, 0)

  private val MinSecond = -31557014167219200L
  private val MaxSecond = 31556889864403199L
  private val MaxNanosInSecond = 999999999
-
+  private val MaxYear = 1000000000
+  private val MinYear = -1000000000
  /*


Missing blank line above this line.

Still relevant.

sjrd · 2018-01-16T15:17:10Z

src/main/scala/java/time/Instant.scala

+  private def handleSegmentParse(segment: String)(classifier: String) = {
+    Try(segment.toInt).getOrElse(
+      throw new DateTimeParseException(s"$segment is not a valid $classifier")
+    )


Coding style: either

Try(segment.toInt).getOrElse { throw new DateTimeParseException(s"$segment is not a valid $classifier") }

or

Try(segment.toInt).getOrElse( throw new DateTimeParseException(s"$segment is not a valid $classifier"))

sjrd · 2018-01-16T15:18:18Z

src/main/scala/java/time/Instant.scala

+    )
+
+    val monthDay = MonthDay.of(month, day)
+    if(monthDay.getMonth == Month.FEBRUARY && leapYear) {


Coding style: space between if and (

sjrd · 2018-01-16T15:47:22Z

src/main/scala/java/time/format/DateTimeParseException.scala

+
+  // TODO
+  // def getErrorIndex(): Int
+  //def getParsedString(): String = parsedData.toString


I see no reason not to implement those trivial methods.

sjrd · 2018-01-16T15:54:51Z

testSuite/shared/src/test/scala/org/scalajs/testsuite/javalib/time/InstantTest.scala

+    assertEquals(Instant.parse("1999-06-03T06:56:23.942Z"), somePositiveInstant)
+    assertEquals(Instant.parse("-0687-08-07T23:38:33.088936253Z"), someNegativeInstant)
+
+


Double blank line.

You should also test one non-String CharSequence, e.g., a java.nio.CharBuffer.wrap(...). Same in LocalDateTest.

sjrd · 2018-01-16T15:56:57Z

testSuite/shared/src/test/scala/org/scalajs/testsuite/javalib/time/LocalDateTest.scala

+    expectThrows(classOf[DateTimeParseException], parse("0000-01-99"))
+    expectThrows(classOf[DateTimeParseException], parse("0000-01-900"))
+    expectThrows(classOf[DateTimeParseException], parse("10000-01-30"))
+


Spurious blank line.

I feel like this is under-tested. For example, I would definitely want to see exceptional cases for non-digits, for Feb 29 on a non-leap year, and stuff like that.

Indeed! In fact it looks like the previous version wouldn't catch the case of a Fed 29 on a non-leap year.

sjrd · 2018-01-16T16:03:48Z

testSuite/shared/src/test/scala/org/scalajs/testsuite/javalib/time/InstantTest.scala

+    expectThrows(classOf[DateTimeParseException], Instant.parse("+1000000001-12-31T23:59:59.999999999Z"))
+    expectThrows(classOf[DateTimeParseException], Instant.parse("-0687-99-07T23:38:33.088936253Z"))
+    expectThrows(classOf[DateTimeParseException], Instant.parse("-ABCD-08-07T23:38:33.088936253Z"))
+  }


The exceptional cases are under-tested here too. For example, what about ...T24:... or T13:65:...?

sjrd · 2018-01-16T16:05:11Z

src/main/scala/java/time/LocalDate.scala

+
+  def parse(text: CharSequence): LocalDate = {
+
+    val potentialLocalDate = Try {


Similar comments as those for Instant.parse() apply here.

chrisshafer · 2018-02-07T22:00:09Z

Hey @sjrd Thanks for the review! Let me know if there is anything else I missed!

sjrd

Thanks. I have a couple more comments.

As I'm leaving on vacation for 3 weeks starting tomorrow, I probably won't have time to look at this PR again before the week of March 5-9.

sjrd · 2018-02-09T10:58:51Z

src/main/scala/java/time/Instant.scala

+
+    println(hi)
+    println(date.getYear)
+    println(years)


Leftover printlns :)

Oh my... Sorry I totally missed these.

sjrd · 2018-02-09T10:58:58Z

src/main/scala/java/time/Instant.scala

    }

+
+    println(yearSegment)


Another one here.

sjrd · 2018-02-09T11:00:41Z

src/main/scala/java/time/Instant.scala

  final val EPOCH = new Instant(0, 0)

  private val MinSecond = -31557014167219200L
  private val MaxSecond = 31556889864403199L
  private val MaxNanosInSecond = 999999999
-
+  private val MaxYear = 1000000000
+  private val MinYear = -1000000000
  /*


Still relevant.

sjrd · 2018-02-09T12:52:29Z

testSuite/shared/src/test/scala/org/scalajs/testsuite/javalib/time/LocalDateTest.scala

+//    expectThrows(classOf[DateTimeParseException], parse("0000-01-900"))
+//    expectThrows(classOf[DateTimeParseException], parse("aaaa-01-30"))
+//    expectThrows(classOf[DateTimeParseException], parse("2012-13-30"))
+//    expectThrows(classOf[DateTimeParseException], parse("2012-01-34"))


Why are those commented out?

sjrd · 2018-02-09T13:04:00Z

src/main/scala/java/time/Instant.scala

+      LocalDate.of(extremeLeapYear * -1, month, day).toEpochDay - epochDaysToAccountForExtreme
+    } else {
+      LocalDate.of(year, month, day).toEpochDay
+    }


The question still stands ;)

chrisshafer · 2018-02-09T15:31:20Z

Ok, thanks for the heads up!

sjrd

Thanks for the updates. I've reformatted the code for you, so that it's good to merge, now.

chrisshafer mentioned this pull request Nov 16, 2017

Instant.toString() does not correctly serialize years. #35

Closed

chrisshafer closed this Jan 10, 2018

chrisshafer reopened this Jan 15, 2018

sjrd requested changes Jan 16, 2018

View reviewed changes

sjrd requested changes Feb 9, 2018

View reviewed changes

Implement Instant.parse and LocalDate.parse.

94b5579

sjrd approved these changes Mar 28, 2018

View reviewed changes

sjrd changed the title ~~LocalDate and Instant parsers~~ Implement Instant.parse and LocalDate.parse. Mar 28, 2018

sjrd merged commit 24f76a4 into scala-js:master Mar 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Instant.parse and LocalDate.parse. #36

Implement Instant.parse and LocalDate.parse. #36

chrisshafer commented Nov 16, 2017

chrisshafer commented Nov 16, 2017

sjrd commented Nov 16, 2017

chrisshafer commented Nov 16, 2017

chrisshafer commented Dec 16, 2017

sjrd commented Dec 17, 2017

chrisshafer commented Jan 15, 2018 •

edited

Loading

sjrd left a comment

sjrd Jan 16, 2018

sjrd Jan 16, 2018

sjrd Jan 16, 2018

sjrd Feb 9, 2018

sjrd Jan 16, 2018

sjrd Jan 16, 2018

sjrd Jan 16, 2018

sjrd Jan 16, 2018

sjrd Jan 16, 2018

sjrd Jan 16, 2018

sjrd Jan 16, 2018

chrisshafer Feb 7, 2018

sjrd Jan 16, 2018

sjrd Jan 16, 2018

chrisshafer commented Feb 7, 2018

sjrd left a comment

sjrd Feb 9, 2018

chrisshafer Feb 9, 2018

sjrd Feb 9, 2018

sjrd Feb 9, 2018

sjrd Feb 9, 2018

sjrd Feb 9, 2018

chrisshafer commented Feb 9, 2018

sjrd left a comment

		assertEquals(Instant.parse("1999-06-03T06:56:23.942Z"), somePositiveInstant)
		assertEquals(Instant.parse("-0687-08-07T23:38:33.088936253Z"), someNegativeInstant)


		def parse(text: CharSequence): LocalDate = {

		val potentialLocalDate = Try {

Implement Instant.parse and LocalDate.parse. #36

Implement Instant.parse and LocalDate.parse. #36

Conversation

chrisshafer commented Nov 16, 2017

chrisshafer commented Nov 16, 2017

sjrd commented Nov 16, 2017

chrisshafer commented Nov 16, 2017

chrisshafer commented Dec 16, 2017

sjrd commented Dec 17, 2017

chrisshafer commented Jan 15, 2018 • edited Loading

sjrd 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

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

chrisshafer commented Feb 7, 2018

sjrd 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

chrisshafer commented Feb 9, 2018

sjrd left a comment

Choose a reason for hiding this comment

chrisshafer commented Jan 15, 2018 •

edited

Loading