JSON Class Hierarchy #2936
JSON Class Hierarchy #2936
Conversation
| case format | ||
| when "json" | ||
| JSONHierarchyPrinter.new(program, exp).execute | ||
| else |
There was a problem hiding this comment.
The else branch should rather abort and tell the user about the invalid parameter argument.
|
Overall looks good. A nitpick of mine would be to avoid generating these intermediate hash and directly stream the JSON to the output. Check the example here, you basically want to pass an IO to the printer, STDOUT, and then append to it as you traverse the types, using And finally, run |
bmulvihill
force-pushed
the
json-hierarchy
branch
4 times, most recently
from
aaee042 to
f7c3ee1
Compare
|
@asterite I made some changes and am now directly streaming to JSON to the output. I was attempting to push the json_objects into json_arrays, but ran into some issues, instead I am now manually creating the arrays within the io stream. Let me know if this is acceptable, thanks! |
bmulvihill
force-pushed
the
json-hierarchy
branch
from
f7c3ee1 to
e3d69b5
Compare
| compute_targets(@program.types, exp, false) | ||
| end | ||
|
|
||
| json = String.build do |io| |
There was a problem hiding this comment.
From here, to the end of the method, this should be:
STDOUT.json_object do |json_object|
print_type(@program.object, json_object, STDOUT)
endotherwise you are streaming to a string in memory, thus creating an object, which is what we wanted to avoid in the first place.
That won't compile because of the type restrictions bellow (JSON::ObjectBuilder(String::Builder)). Just remove those restrictions, they aren't necessary here. Then it will compile.
|
@bmulvihill Looks really good, thanks! It's true, it's a bit hard to use I've made a few more comments, these should be the last ones. |
bmulvihill
force-pushed
the
json-hierarchy
branch
from
e3d69b5 to
c422d52
Compare
|
@asterite I made more changes. I had to do STDOUT.flush after streaming, otherwise I was getting EOF errors. Is this expected, or was I missing something? |
|
@bmulvihill Thanks, I'll soon take a look. In my tests I didn't need to flush, I'll probably merge your work and fix anything that I find, if I find anything :-) |
|
@bmulvihill Let's do the flush, the error you get is #2961 |
Based on #1387
Allows a user to pass in -f parameter when using the hierarchy tool and obtain JSON class hierarchy