Conversation
| | private def toJsonValue(value: Any): String = { | ||
| | value match { | ||
| | case elem: Seq[_] => elem.map(toJsonValue).mkString("[", ",", "]") | ||
| | case elem: Option[_] => elem.map(toJsonValue).orNull |
There was a problem hiding this comment.
I don't think we should return null. I think returning "null" was correct. WDYT?
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"
}
}There was a problem hiding this comment.
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.
There was a problem hiding this comment.
No, I get null on line 71 where we work with Option.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sorry, maybe I missed something. Can you explain by example, why you want to return "quoted null"
There was a problem hiding this comment.
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. 😄
| | value match { | ||
| | case elem: Seq[_] => elem.map(toJsonValue).mkString("[", ",", "]") | ||
| | case elem: Option[_] => elem.map(toJsonValue).orNull | ||
| | case elem: Map[String, Any] => elem.map { |
There was a problem hiding this comment.
(Minor - feature creep): We could also match on Map[String, Any] and sending the key through toJsonValue here. IIUC
There was a problem hiding this comment.
Yes, I think we can do it
# Conflicts: # project/build.properties
|
Pull request updated )) |
Now if have the next keys
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.