Open ambrusadrianzh opened 5 years ago
Agreed, add
and merge
should only work on JsObjects
.
However, I'm not sure if add
and merge
serve the same purpose. With add
you can add JsString
, JsNumber
etc. to a JsObject
with a certain key.
The merge
function should merge two JsObjects together. This way we can avoid the following confusing situtations:
val a = JsObject("weight" to JsFloat(61.4f))
val b = JsString("abc")
val c = a.merge(b) // c == b for no reason
If we constrain the merge function to JsObjects only, these situations will not able to happen. I'm happy to make these changes, if we can agree on these.
As it is on this PR, merge
serves the same purpose than add
because if, continuing with your example, a.merge(b)
doesn't do anything because b
has to be a JsObject
.
You can restrict that making merge
to receive a JsObject
directly and it would be the same as add
with the insignificant difference that add
just allow you to add new values one by one and merge
would allow you to add them all at the same call.
I agree to move them to JsObject
but I would like the opinion of @nomisRev here too
@AdrianRaFo @nomisRev How should we proceed? 🤔
Hey @nomisRev,
Sorry for the delay. Made the changes today, caught up with upstream and integrated the testing changes from arrow test. Please take a look at it, as I'm not sure I got it right.
@ambrusadrianzh no worries! Thank you for the contributions! I saw @AdrianRaFo already checked out your PR, I can give it a look next week. This week is a little busy with travelling and KotlinConf ;)
Fixes #118
Changes: