-
Notifications
You must be signed in to change notification settings - Fork 10
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 macro to check for required json keys #112
base: master
Are you sure you want to change the base?
Add macro to check for required json keys #112
Conversation
Should not return false if the value is null, as long as the key exists
oh @DevwratJoshi actually you might need to port controllercommon!488 as well. |
ask Yoshiki if required |
@@ -37,6 +37,15 @@ | |||
#include <rapidjson/error/en.h> | |||
#include <rapidjson/prettywriter.h> | |||
|
|||
#ifndef MUJINCLIENTJSON_LOAD_REQUIRED_JSON_VALUE_BY_KEY | |||
#define MUJINCLIENTJSON_LOAD_REQUIRED_JSON_VALUE_BY_KEY(rValue, key, param) \ |
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.
Is there a reason this is a macro instead of a function LoadRequiredJsonValueByKey
?
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.
ya, actually I also think function is better, then we can put it in a namespace...
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.
should we talk in controllercommon!489 ?
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 see, Ziyan brought up the same concern here. Let's keep discussion here for now.
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.
That makes you, me and @ziyan arguing for a function over a macro then
@cielavenir Yoshiki said that the changes from controllercommon!488 could be better, so they might be changed in the future. By the way, the behaviour in master for |
My understanding is that Webstack will not distinguish between fields that are unset and fields that are set to cc @ziyan |
Does anyone have any issues with this change? I don't. |
LoadJsonValueByKey
now returns a bool. If the key does not exist in the document the function will return false, else will return true.MUJINCLIENTJSON_LOAD_REQUIRED_JSON_VALUE_BY_KEY
) is added, wrappingLoadJsonValueByKey
. If the keys are not in the document, an exception is thrown.