-
Notifications
You must be signed in to change notification settings - Fork 29
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
remove Json.apply() methods and simplify Json.arr() usage
- Loading branch information
Showing
2 changed files
with
24 additions
and
81 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
10f8fa9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the rationale behind the removal of
Json.apply()
?10f8fa9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed it here. We made it more explicit from a factory method perspective whether you are creating a JSON object or array. Did you prefer the old way with apply() as central factory method? Or where you using JSON objects for some pattern matching cases?
10f8fa9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just didn't see why
Json.obj(...)
was suddenly preferred to the shorterJson(...)
On a side note, I also needed a few minutes to realize that I needed to change my
Json.arr(someSeq)
toJson.arr(someSeq: _*)
after this change.10f8fa9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tsuna The problem is that you'd never really know if you use
apply()
forJsonObject
orJsonArray
. If we'd allow it, then only for one kind and which one should we prefer? ProbablyJsonObject
, but people reading the code will always have to think about that first. Writing these 4 characters to make it explicit is probably better.I saw the
Json.arr(someSeq: _*)
too and am trying to mitigate this by matching for Sequences and directly converting them intoJsonArray
s if you're inJson.obj(...)
orJson.arr(...)
. But I still have a problem when you have a recursive List, for exampleJson.obj("some_list" -> List(1, 2, List(3, 4)))
->{ "some_list" : [1, 2, []] }
. Need to check that and I'll add something so working with lists will be easier again.Sorry for the inconvenience regarding this API breaking, but we're still not at 1.0 and we're trying to find the best things for users. If you have suggestions, please shoot :)
10f8fa9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed my problem (stupid me) and made a PR: #82
10f8fa9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Narigo Maybe, in order to follow the traditional method of using apply() as factory method (instead of
arr
, orobj
names), you could have two companion objects, one for JsonObject and one for JsonArray, and then in each have an apply() method that takes the expected parameters? I think this would be more intuitive:You'd loose the methods in Json, but i don't think that's bad. People are already having to decide between
obj
orarr
, which is the same as chosing betweenJsonArray()
andJsonObject()
Thoughts?
10f8fa9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@galderz We could provide both,
JsonObject.apply(str: String) = new JsonObject(str)
andJsonArray.apply(str: String) = new JsonArray(str)
. I somehow like the shorterJson.arr
.Json.obj
form, especially as you canimport Json._
and use them directly in the very short form ofarr()
andobj()
.10f8fa9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Narigo I had a look at other Json libraries (i.e. Play! 2) and they don't use
apply()
as factories, so let's leave it as is. I'd like our methods to be consistent though in their naming:fromArrayString
fromObjectString
emptyArr
emptyObj
obj
arr
addToArray
addToObject
^ I think all methods should say the full word
array
orobject
to be consistent. So,emptyArray
,emptyObject
,array
,object
. Thoughts?10f8fa9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I ever used
fromArrayString
orfromObjectString
. Guess anapply(json: String): JsonElement
where aJsonElement
can only be aJsonArray
orJsonObject
would be a better idea. AreaddToArray
andaddToObject
public? Is there a reason why?If you want to change
arr
andobj
, I don't mind too much - but we'd have to change all the client code again....and I admit I kind of like the piratey feeling I got from writing
arr
all the time. ;)10f8fa9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't that lead to casting on the client side?
Right now we are relying on the Java JSON classes, we should probably have our own
JsonArray
andJsonArray
to which you can add/remove items.Yeah, i think I'd rather have full names, particularly for small names such as
array
andobject
. I'll add an issue for this and I owe you a beer ;)