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.

Sign up for GitHub

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

Correct json for nested objects #126

Merged
merged 4 commits into from
Jul 26, 2018
Merged

Correct json for nested objects #126

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1a1498f
allow to make correct json for nested objects
yarosman May 24, 2018
9aa839b
use toJsonValue for key representation too
yarosman May 24, 2018
546fe6d
Merge remote-tracking branch 'upstream/master'
yarosman Jul 26, 2018
a830a07
update json creation methods
yarosman Jul 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion project/build.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
sbt.version=1.1.1
sbt.version=1.1.5
22 changes: 13 additions & 9 deletions src/main/scala/sbtbuildinfo/ScalaCaseObjectRenderer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,16 +63,20 @@ private[sbtbuildinfo] case class ScalaCaseObjectRenderer(options: Seq[BuildInfoO
def toJsonLine: Seq[String] =
if (options contains BuildInfoOption.ToJson)
List(
""" val toJson: String = toMap.map{ i =>
| def quote(x:Any) : String = "\"" + x + "\""
| val key : String = quote(i._1)
| val value : String = i._2 match {
| case elem : Seq[_] => elem.map(quote).mkString("[", ",", "]")
| case elem : Option[_] => elem.map(quote).getOrElse("null")
| case elem => quote(elem)
""" private def quote(x: Any): String = "\"" + x + "\""
|
| private def toJsonValue(value: Any): String = {
| value match {
| case elem: Seq[_] => elem.map(toJsonValue).mkString("[", ",", "]")
| case elem: Option[_] => elem.map(toJsonValue).orNull
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should return null. I think returning "null" was correct. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't agree, we try to convert to correct json type and null is not "null".
For example, we parse BuildInfo.toJson and instead of getting for some value Option[String] we get String.

https://www.json.org/
"A value can be a string in double quotes, or a number, or true or false or null, or an object or an array."

Copy link
Member

Choose a reason for hiding this comment

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

I see what you mean. We are converting to the correct JSON type, but the string representation of that type.

In that case string representation of JSON null is "null", no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At final we get something like that

{
  "build_number": "1.0-SNAPSHOT",
  "git": {
    "commit": null,
    "commit_date": "2018-05-15T15:48:43+0300",
    "tags": [
      
    ],
    "commit_branch": "develop"
  }
}

Copy link
Member

@dwijnand dwijnand May 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

So that works because (on line 73) the null you're returning right now is passed to String#+

And "commit" + ":" + null == "commit:null".

So it's kind of "accidentally doing the right thing". But I'm fairly certain we be returning "null" here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I get null on line 71 where we work with Option.

Copy link
Member

Choose a reason for hiding this comment

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

Correct, that's the code the produces the null from the option and gets returned by the call to toJsonValue.

The code calling that toJsonValue is the code at line 73, which is consuming the map you described in the PR description: #126 (comment)

It's only because your passing it to String#+ that it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, maybe I missed something. Can you explain by example, why you want to return "quoted null"

Copy link
Member

Choose a reason for hiding this comment

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

I want to return the string representation of JSON null.

Here, these examples might help:

  • toJsonValue(Some(123)) should return "123"
  • toJsonValue(Some("abc")) should return "\"abc\"" (quoted string)
  • toJsonValue(None) should return "null"

Another addition we could do is make toJsonValue(null) return "null".

Sorry, the fact that the code is recursive and there's multiple levels of strings makes this hard to communicate. 😄

| case elem: Map[String, Any] => elem.map {
Copy link
Member

Choose a reason for hiding this comment

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

(Minor - feature creep): We could also match on Map[String, Any] and sending the key through toJsonValue here. IIUC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think we can do it

| case (k, v) => quote(k) + ":" + toJsonValue(v)
| }.mkString("{", ", ", "}")
| case other => quote(other)
| }
| s"$key : $value"
| }.mkString("{", ", ", "}")""".stripMargin)
| }
|
| val toJson: String = toJsonValue(toMap)""".stripMargin)
else Nil

}
22 changes: 13 additions & 9 deletions src/sbt-test/sbt-buildinfo/options/build.sbt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,16 +41,20 @@ lazy val root = (project in file(".")).
""" "name" -> name,""" ::
""" "scalaVersion" -> scalaVersion)""" ::
"""""" ::
""" val toJson: String = toMap.map{ i =>""" ::
""" def quote(x:Any) : String = "\"" + x + "\""""" ::
""" val key : String = quote(i._1)""" ::
""" val value : String = i._2 match {""" ::
""" case elem : Seq[_] => elem.map(quote).mkString("[", ",", "]")""" ::
""" case elem : Option[_] => elem.map(quote).getOrElse("null")""" ::
""" case elem => quote(elem)""" ::
""" private def quote(x: Any): String = "\"" + x + "\""""" ::
"""""" ::
""" private def toJsonValue(value: Any): String = {""" ::
""" value match {""" ::
""" case elem : Seq[_] => elem.map(toJsonValue).mkString("[", ",", "]")""" ::
""" case elem : Option[_] => elem.map(toJsonValue).orNull""" ::
""" case elem: Map[String, Any] => elem.map {""" ::
""" case (k, v) => quote(k) + ":" + toJsonValue(v)""" ::
""" }.mkString("{", ", ", "}")""" ::
""" case other => quote(other)""" ::
""" }""" ::
""" s"$key : $value"""" ::
""" }.mkString("{", ", ", "}")""" ::
""" }""" ::
"""""" ::
""" val toJson: String = toJsonValue(toMap)""" ::
"""}""" :: Nil =>
case _ => sys.error("unexpected output: \n" + lines.mkString("\n"))
}
Expand Down