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

Provide play-json <-> circe conversions #28

Merged
merged 7 commits into from
Jan 31, 2023

Conversation

efemelar
Copy link
Contributor

No description provided.

jsonObject = o => PlayJson.JsObject(o.toIterable.map { case (k, v) => (k, circeToPlay(v)) }.toSeq),
)

def playToCirce(value: PlayJson.JsValue): CirceJson =
Copy link
Contributor

Choose a reason for hiding this comment

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

tailrec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

circeToPlay is not in tail position

Copy link
Contributor

Choose a reason for hiding this comment

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

this is a question - we either decide that we are not going to support ~1000 nested elements or try to rewrite so it could be tailrec-ed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I don't know.

It's another part which was not changed compared to original solution and worked so far without blowing the stack.

If we keep it, then both these conversions deserve disclaimer about safety in scaladoc.

Copy link

@jurisk jurisk Jan 31, 2023

Choose a reason for hiding this comment

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

Suggestion:

  @tailrec
  private def playToCirceSeq(seq: scala.collection.IndexedSeq[PlayJson.JsValue], acc: List[CirceJson] = Nil): Iterable[CirceJson] = {
    seq.headOption match {
      case Some(value) => 
        playToCirceSeq(seq.tail, playToCirce(value) :: acc)
      case None        => 
        acc.reverse
    }
  }

  @tailrec
  private def playToCirceKeyValue(list: List[(String, JsValue)], acc: List[(String, CirceJson)] = Nil): Iterable[(String, CirceJson)] = {
    list match {
      case head :: tail =>
        val (k, v) = head
        playToCirceKeyValue(tail, (k, playToCirce(v)) :: acc)
      case Nil          =>
        acc.reverse
    }
  }

  def playToCirce(value: PlayJson.JsValue): CirceJson = {
    value match {
      case PlayJson.JsNull =>
        CirceJson.Null

      case PlayJson.JsTrue =>
        CirceJson.True

      case PlayJson.JsFalse =>
        CirceJson.False

      case PlayJson.JsNumber(value) =>
        CirceJson.fromBigDecimal(value)

      case PlayJson.JsString(value) =>
        CirceJson.fromString(value)

      case PlayJson.JsArray(value) =>
        CirceJson.fromValues(playToCirceSeq(value))

      case PlayJson.JsObject(value) =>
        CirceJson.fromFields(playToCirceKeyValue(value.toList))
    }
  }

Data structures used could be changed (e.g. to avoid List.reverse).

@coveralls
Copy link

coveralls commented Jan 31, 2023

Coverage Status

Coverage: 41.889% (+9.2%) from 32.696% when pulling e9461b3 on efemelar:to-from-circe-conversions into 8f94781 on evolution-gaming:master.

@t3hnar t3hnar merged commit 1131eec into evolution-gaming:master Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants