Skip to content
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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

DevwratJoshi
Copy link

@DevwratJoshi DevwratJoshi commented Jun 14, 2022

  1. LoadJsonValueByKey now returns a bool. If the key does not exist in the document the function will return false, else will return true.
  2. A macro (MUJINCLIENTJSON_LOAD_REQUIRED_JSON_VALUE_BY_KEY) is added, wrapping LoadJsonValueByKey. If the keys are not in the document, an exception is thrown.

@DevwratJoshi DevwratJoshi marked this pull request as ready for review June 14, 2022 05:42
Should not return false if the value is null, as long as the key exists
@cielavenir
Copy link

oh @DevwratJoshi actually you might need to port controllercommon!488 as well.

@cielavenir
Copy link

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) \
Copy link
Contributor

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?

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...

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 ?

Copy link
Contributor

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.

Copy link
Contributor

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

@DevwratJoshi
Copy link
Author

DevwratJoshi commented Jun 14, 2022

@cielavenir Yoshiki said that the changes from controllercommon!488 could be better, so they might be changed in the future.
We decided that it is best to be consistent across the 3 repos as far as possible though, so I have added the changes from that MR to this one as well. If we change that code block in one of the repos in the future, we will have to be sure to change all of them.

By the way, the behaviour in master for LoadJsonValueByKey is to return false if the key is present but the value is null (which will also throw an exception if the function is called via MUJINCLIENTJSON_LOAD_REQUIRED_JSON_VALUE_BY_KEY.
Do you think this is ok? We are basically assuming that required keys cannot have Null value.

@felixvd
Copy link
Contributor

felixvd commented Jun 14, 2022

By the way, the behaviour in master for LoadJsonValueByKey is to return false if the key is present but the value is null (which will also throw an exception if the function is called via MUJINCLIENTJSON_LOAD_REQUIRED_JSON_VALUE_BY_KEY.
Do you think this is ok? We are basically assuming that required keys cannot have Null value.

My understanding is that Webstack will not distinguish between fields that are unset and fields that are set to null, and this behavior is in line with that.

cc @ziyan

@rdiankov
Copy link
Member

rdiankov commented Nov 3, 2022

Does anyone have any issues with this change? I don't.
Also, can we get the same change in the internal mujinjson.h file?
Thanks,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants