-
Notifications
You must be signed in to change notification settings - Fork 506
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
Add support of new command JSON.SET and JSON.GET #1803
Conversation
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.
LGTM, thanks for your great work!
We can create an issue to track other new commands. And others can help to implement the new commands if they like. |
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.
General it would be a good point to start, so just some nits.
jsoncons::json val; | ||
|
||
try { | ||
val = jsoncons::json::parse(str); |
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 think we'd better use parse
with erroc
version to avoid costing from the exception.
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.
It will be fixed in later PRs, or you can pick up it if you have free time : )
jsoncons::jsonpath::json_replace(value, path, [&new_value](const std::string & /*path*/, jsoncons::json &origin) { | ||
origin = std::move(new_value.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.
Assume the path matches mutlple sub json object, this will be wrong
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 think since it follows preorder traversal so it will goes well when there are mutlple sub json objects matched in the original object.
But it will be wrong if there are json objects matched in the new_value, which will be fixed later.
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 so, since new_value.value
has been moved.
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, good point. I will fix it.
Tests will be added in further PRs to make an example/reference for other issues in the tracking issue : ) |
We add support of two new commands
JSON.SET
andJSON.GET
(despite some options missing).Besides, we add two new methods
IsSingleKVType
andIsEmptyableType
to classMetadata
for more intuitive coding.JSON Metadata encoding:
The field
format
is to make JSON data type more extensible. Currently it can only be0
to indicate that the data stored inpayload
is in the JSON format.TODO: