Skip to content
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

implementing query parameter encoder/decoders (LocalDate, ZonedDate) #3634

Closed
wants to merge 10 commits into from
Closed

Conversation

jyoo980
Copy link
Contributor

@jyoo980 jyoo980 commented Aug 8, 2020

Was looking through some issues when I found this conversation in #2670:

@LukaJCB - #2724 added support for Instant. What other types would you find useful - ZonedDateTime and LocalDate?

In this PR, I added support for LocalDate. I have something in my fork for ZonedDateTime, but maybe that'll come later since I'm having trouble testing it correctly.

I hope this is a useful contribution!

@jyoo980 jyoo980 changed the title implementing QueryParamEncoder[LocalDate] and QueryParamDecoder[LocalDate] implementing query parameter encoder/decoders (LocalDate, ZonedDate) Aug 8, 2020
@jyoo980
Copy link
Contributor Author

jyoo980 commented Aug 8, 2020

Looks like there are dependency/compilation issues for adding ZonedDateTime, fails for anything below Scala 2.13 and anything other than JDK 1.8, maybe we can exclude this decoder/encoder for now?

I suspect that this is due to way that Java to Scala conversions are done for collection types. The 2.12 way of doing it is deprecated in 2.13, while the way it's done in 2.13 via the scala.jdk.CollectionConverters (which is what I'm doing now), isn't supported in earlier versions.

@rossabaker
Copy link
Member

rossabaker commented Aug 15, 2020

Sorry, I started reviewing this, and I don't know where it ended up.

We have an import org.http4s.internal.CollectionCompat.CollectionConverters._. If you need to bridge the gap between the collections API in other ways, they can be added to CollectionCompat in the two source directories, but I think that will fix the remaining issue.

Otherwise, looks good!

@rossabaker
Copy link
Member

rossabaker commented Aug 15, 2020

It also looks binary compatible, so we can cherry-pick this back to 0.21.

@rossabaker rossabaker added enhancement Feature requests and improvements retarget Cherry-pick or reopen on another branch labels Aug 15, 2020
@jyoo980
Copy link
Contributor Author

jyoo980 commented Aug 15, 2020

Thanks for the feedback, I'll work on this over my boring weekend 😉

@jyoo980
Copy link
Contributor Author

jyoo980 commented Aug 15, 2020

Looks like a Flakier than it's worth on CI test failed on the latest run. Other than that, looks like the tests related for my change passed.

(start + math.random() * diff).toLong
val zoneIds: Seq[String] = ZoneId.getAvailableZoneIds.asScala.toSeq
Arbitrary {
for {
Copy link
Contributor Author

@jyoo980 jyoo980 Aug 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly embarrassed that I didn't think of this sooner, much better than

  implicit val ArbitraryZonedDateTime: Arbitrary[ZonedDateTime] = {
    // Can't use Random.between because it breaks compatibility with
    // anything below Scala 2.13
    def randBetween(start: Long, end: Long): Long = {
      val diff = end - start
      (start + math.random() * diff).toLong
    }

    val zoneIdStrs = ZoneId.getAvailableZoneIds.asScala.toSeq
    Arbitrary(Gen.oneOf[String](zoneIdStrs).map { zoneIdStr =>
      val zoneId: ZoneId = ZoneId.of(zoneIdStr)
      val secSinceEpoch: Long = randBetween(Instant.MIN.getEpochSecond, Instant.MAX.getEpochSecond)
      ZonedDateTime.ofInstant(Instant.ofEpochSecond(secSinceEpoch), zoneId)
    })
  }

Copy link
Member

@rossabaker rossabaker left a comment

Looks good. I'm pretty sure it's binary compatible, so I'm going to try to bring this back to series/0.21 instead of merging, so it can get in the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests and improvements retarget Cherry-pick or reopen on another branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants