Correct json for nested objects

[yarosman]

Now if have the next keys

buildInfoKeys := Seq[BuildInfoKey.Entry[_]](
  "build_number" -> version.value,
  "git" -> Map(
    "commit" -> git.gitHeadCommit.value,
    "commit_date" -> git.gitHeadCommitDate.value,
    "tags" -> git.gitCurrentTags.value,
    "commit_branch" -> git.gitCurrentBranch.value
  )
)

we will get the next json:

{"build_number" : "1.0-SNAPSHOT", "git" : "Map(commit -> Some(67858ae976394cfd6dfe27336905458a8412d383), commit_date -> Some(2018-05-15T15:48:43+0300), tags -> List(), commit_branch -> develop-commons)"}

So we have wrong representation for nested Map object.
This merge request allow to generate more correctly json for nested objects.

[dwijnand]

Nice PR, @yarosman!

I have a concern around the change to matching Option, and a (less important) idea around matching Map.

But other than that this looks good to go to me!

[dwijnand]

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

[yarosman]

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."

[dwijnand]

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?

[yarosman]

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"
  }
}

[dwijnand]

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.

[yarosman]

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

[dwijnand]

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.

[yarosman]

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

[dwijnand]

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. 😄

[dwijnand]

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

[yarosman]

Yes, I think we can do it

[yarosman]

Pull request updated ))

[dwijnand]

Looks good. Thanks @yarosman.