Add --json flag in frame to provide JSON output support #116

suprith-hub · 2024-07-02T16:41:01Z

@jviotti I have used brute force way to pad spaces to indent the JSON output lines. Should that be done differently?
Please review my PR

jviotti · 2024-07-02T17:30:28Z

It's starting to shape up, but you should output the entire thing as one big JSON document, rather than parts of it as JSON. Then you wouldn't need to worry about indentation at all.

jviotti · 2024-07-02T17:31:30Z

src/command_frame.cc

+    for (const auto &[key, entry] : frame) {
+        std::cout << "(LOCATION) URI: ";
+        std::cout << key.second << "\n";
+        std::cout << "    Schema           : " << entry.root.value_or("<ANONYMOUS>")


i.e. all of these things you should be transforming to JSON. In the end you want the command to print only one big JSON document with all the information at once

Ohh, I got it. I thought it might be this way, just in sometime...

jviotti · 2024-07-07T18:40:10Z

src/command_frame.cc

-    std::cout << "    Pointer          :";
-    if (!entry.pointer.empty()) {
-      std::cout << " ";
+const char* enumToString(sourcemeta::jsontoolkit::ReferenceEntryType type) {


You should never return a const char *. Please return an std::string

Also, in C++, we don't use camelCase, but snake_case in variables and function calls. Please do enum_to_string or something like that. Same applies to various other parts in this PR

Also, in modern C++, we use trailing return type style. i.e. auto enum_to_string(...) -> result

Finally, this function doesn't modify its argument, so you want to make the argument const

Also not strictly necessary in this case, but as a good practice, given this is a private function, please make it static

Cool! I got it

jviotti · 2024-07-07T18:42:21Z

src/command_frame.cc

-    std::cout << "(REFERENCE) URI: ";
-    sourcemeta::jsontoolkit::stringify(pointer.second, std::cout);
-    std::cout << "\n";
+    bool outputJson = options.contains("json") || options.contains("j");


All variables must be camel_case

Also, try to use const whenever possible. This variable is not supposed to change, so it should be a constant

jviotti · 2024-07-07T18:42:50Z

src/command_frame.cc

-    std::cout << "\n";
+    bool outputJson = options.contains("json") || options.contains("j");
+    if (outputJson) {
+        sourcemeta::jsontoolkit::JSON outputJson = sourcemeta::jsontoolkit::JSON::make_object();


You can do auto here

Isn't specifying type always a good choice.. Why should we use auto in this case..?

You already are kinda specifying the type on the right hand side of assignment (sourcemeta::jsontoolkit::JSON from sourcemeta::jsontookit::JSON::make_object), so this is a bit repetitive. That's why I suggested auto.

jviotti · 2024-07-07T18:43:44Z

src/command_frame.cc

@@ -1,71 +1,112 @@
 #include <sourcemeta/jsontoolkit/json.h>
 #include <sourcemeta/jsontoolkit/jsonschema.h>
-


Why was this line deleted?

jviotti · 2024-07-07T18:44:27Z

docs/frame.markdown

@@ -58,3 +58,9 @@ reference:
 ```sh
 jsonschema frame path/to/my/schema.json
 ```
+
+### Frame a JSON Schema with output in the form of JSON


Suggested change

### Frame a JSON Schema with output in the form of JSON

### Frame a JSON Schema and output result as a JSON document

jviotti · 2024-07-07T18:45:06Z

CI is complaining about formatting (i.e. https://github.com/Intelligence-AI/jsonschema/actions/runs/9825814598/job/27134677359?pr=116). Try running make while having ClangFormat installed to re-format the code

jviotti · 2024-07-07T18:45:55Z

Also, can you add a variant of this test (https://github.com/Intelligence-AI/jsonschema/blob/main/test/frame.sh) to exercise the new JSON functionality?

suprith-hub · 2024-07-10T18:21:16Z

@jviotti status says still waiting for approval

jviotti · 2024-07-11T19:31:04Z

@Era-cell Try now. I tweaked the project settings to be more permissive with first-time contributors

src/command_frame.cc

jviotti · 2024-07-11T19:32:13Z

test/frame.sh

@@ -70,4 +70,4 @@ cat << 'EOF' > "$TMP/expected.txt"
    - (fragment)     : /$defs/string
 EOF

-diff "$TMP/result.txt" "$TMP/expected.txt"


This looks like an unintended change

It doesn't have any difference between the original version, still I do not know why it says there is a diff

It's the final new line character at the end of the file. We saw it in other PRs. It's likely a misconfiguration on your editor. Google how to make sure your editor includes an end-of-file line ending

See the symbol that GitHub shows on the diff:

Shet! sorry I got it, I think myself I removed the EOF line, I will correct it

test/frame_json_output.sh

jviotti · 2024-07-11T19:34:09Z

src/command_frame.cc

-      std::cout << " ";
+    for (const auto &[key, entry] : frame) {
+      auto frame_entry = sourcemeta::jsontoolkit::JSON::make_object();
+      frame_entry.assign("schema", sourcemeta::jsontoolkit::JSON{entry.root.value_or("<ANONYMOUS>")});


If the root is not defined, can you make it null rather than setting it to a string with the value of <ANONYMOUS>?

test/frame_json_output.sh

jviotti · 2024-07-11T19:34:39Z

test/frame_json_output.sh

+{
+  "frames": {
+    "https://example.com": {
+      "baseURI": "https://example.com",


Here too. Maybe just call it base?

jviotti · 2024-07-11T19:35:34Z

src/command_frame.cc

+                       sourcemeta::jsontoolkit::JSON{pointer.first == sourcemeta::jsontoolkit::ReferenceType::Dynamic ? "Dynamic" : "Static"});
+      ref_entry.assign("destination", sourcemeta::jsontoolkit::JSON{entry.destination});
+      if (entry.base.has_value()) {
+        ref_entry.assign("fragmentBaseURI", sourcemeta::jsontoolkit::JSON{entry.base.value()});


Here you can make it just base. I think in general keeping the JSON properties consistent with the names in the struct will make it easier to maintain

src/command_frame.cc

suprith-hub · 2024-07-15T07:04:55Z

@jviotti what is the clang formatter I should be using? I used llvm clang with vscode extention, but its still failing

jviotti · 2024-07-15T12:49:10Z

@Era-cell I don't use VS Code, so not sure about that plugin, but you might be installing a different version? Here is how we do it for all platform on GitHub Actions: https://github.com/Intelligence-AI/jsonschema/blob/main/.github/workflows/test.yml#L40-L41

suprith-hub · 2024-07-17T19:14:04Z

@Era-cell I don't use VS Code, so not sure about that plugin, but you might be installing a different version? Here is how we do it for all platform on GitHub Actions: https://github.com/Intelligence-AI/jsonschema/blob/main/.github/workflows/test.yml#L40-L41

@jviotti I did format using clang-format version 18.1.5 Python, it still fails, idk why

jviotti · 2024-07-17T22:50:45Z

Do you get an error, or do you mean the formatting is not happening how you would expect it to happen? How are you running it, specifically? What you get if you run clang-format --version?

suprith-hub · 2024-07-18T21:21:48Z

@jviotti looks like clang formatting worked for windows but not for mac, and ubuntu

jviotti · 2024-07-18T22:07:01Z

Formatting is OK now, it's the test that's failing. We don't run tests on Windows (yet) so that's why that's green.

…e URI instead of Base

suprith-hub · 2024-07-20T16:19:56Z

Ok, its good.. but can you check the changes in the non-json output ...

test/frame/pass_json_output.sh

test/frame/pass.sh

jviotti · 2024-07-22T14:28:11Z

It's looking really good! Just one minor comment and I think we are done

src/command_frame.cc

suprith-hub · 2024-07-23T13:55:20Z

@jviotti I have updated the PR from my side.. can you check it, if I have missed any other case

jviotti · 2024-07-23T14:19:09Z

Thanks a lot! Great work, and I hope you enjoyed C++ :D I'll release a new version with this change soon and announce it on LinkedIn

suprith-hub · 2024-07-23T16:09:00Z

Thanks a lot! Great work, and I hope you enjoyed C++ :D I'll release a new version with this change soon and announce it on LinkedIn

Thank you for guiding me 😁, I took a lot of time 😅...

Frame has --json flag for JSON output support

2cbce79

jviotti reviewed Jul 2, 2024

View reviewed changes

Add frame type and correct the output format but not the output itself

d2bc412

jviotti reviewed Jul 7, 2024

View reviewed changes

Correct best practices, add test for JSON output using -j/-json flags

ae2f7f5