-
-
Notifications
You must be signed in to change notification settings - Fork 324
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
r.describe: Add JSON support #4918
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Nishant Bansal <[email protected]>
Signed-off-by: Nishant Bansal <[email protected]>
Signed-off-by: Nishant Bansal <[email protected]>
Hi everyone, could someone please review this PR when you have a moment? Thank you! |
About the format shown in the body of your PR: Does this mean that the root of the json output is an array instead of an object (like {})? I didn't know it was valid json. I see that the similat shape is used in the test. For your PR, I don't think I'm able to judge about the C part. But for the Python and test part I could check out. |
Yes, the format will be an array of objects, each containing a category value (originally separated by newlines in the plain format). However, if this format is not appropriate, we can discuss and consider a new output format. |
Signed-off-by: Nishant Bansal <[email protected]>
Signed-off-by: Nishant Bansal <[email protected]>
Signed-off-by: Nishant Bansal <[email protected]>
Hey @petrasovaa, I tried following the suggestion from #4903 (comment) with some minor adjustments. Could you take a look? |
That was fast! This looks great, I will test it more tomorrow and let you know if anything else is needed. |
I finally tested it and the only thing I would change is the null reporting, it seems to me that a boolean like "has_nulls" makes more sense here for the JSON, so I would ignore the nv parameter. The test would be normally better done as a separate PR before r.describe is modified, but I tested the plain format parts of the test without the changes so it's fine as it is now. |
Signed-off-by: Nishant Bansal <[email protected]>
fixes: #4903
Use parson to add json output format support to the r.describe module.
The JSON output looks like as follows: