-
Notifications
You must be signed in to change notification settings - Fork 17
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.
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
Reformat docs; minor fixes #244
base: master
Are you sure you want to change the base?
Conversation
@@ -51,7 +50,7 @@ class JsonMergePatch(Doc patch) { | |||
if (Doc targetValue := target.get(key)) { | |||
merge(key, merge(targetValue, value, inPlace)); | |||
} else { | |||
merge(key, merge(json.newObject(), value, True)); | |||
merge(key, merge(json.newObject(), value, True)); // TODO JK: why not "inPlace" |
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.
Here we are merging value
into a new JsonObject
so we may as well do it in place (hence the True
argument). There is no point doing a copy if the inPlace
parameter is False
.
Also, I think line 31 is wrong, but I cannot add a comment to it on that line. I suspect it should be return merge(target, patch, inPlace);
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.
Hmm... looking closer into the
private Doc merge(Doc doc, Doc patch, Boolean inPlace = False)
implementation I realized, that inPlace
argument is not used for anything but a recursive call to itself. The actual value affects absolutely nothing.
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.
Ah, it appears to always be doing an in-place merge, i.e modifying the target directly. Maybe that needs fixing.
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.
Not just that; the "inPlace" flag is not used for anything (e.g. ListMap construction)
* | ||
* @param p the `Primitive` to merge | ||
* @param obj the `Primitive` value to merge (TODO JK: value is not used!?) |
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.
Right, it can be removed.
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.
Hmm... but if we remove it, then what is the point of calling this method? Don't we lose the value?
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.
So you can merge one json object into another, or one json array into another, but you cannot merge anything into a primitive. So if the target field is a primitive, it is basically just replaced with the value being merged in.
e.g. If I have
{"foo": 1234}
so "foo" is a primitive number field. The I want to merge the array [1, 2, 3, 4]
into field "foo" I end up with
{"foo": [1, 2, 3, 4]}
If "foo" was an array
{"foo": [9, 8, 7]}
and I did the same merge then I believe I would get
{"foo": [9, 8, 7, 1, 2, 3, 4]}
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.
And, yes we lose the original value if it was a primitive
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.
Oh... I got confused by the doc then. The method doc says:
Deeply merge a `Primitive` value into this builder.
...
@param doc the value to merge the primitive into
So I thought the current value is supposed to be replaced by the primitive. I think it needs to be improved
0675723
to
2a45369
Compare
I have submitted some updates.
|
Looks good! |
a860300
to
2685fc6
Compare
9e7b0e3
to
9bd5344
Compare
While using JsonPointer API I noticed that some of the docs are formatted with a style different from other classes in this package. This is a trivial change - put there as a draft for JK to review two suspicious places in the code