-
-
Notifications
You must be signed in to change notification settings - Fork 489
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 JSON text renderer #1086
Add JSON text renderer #1086
Conversation
bb820c2
to
760514b
Compare
a1f3814
to
20810b9
Compare
Glad to see JSON rendering is finally supported, after my proposal was rejected 😉 |
@thoemmi It wasn't rejected. We didn't want to add a dependency to a JSON library 😊 |
Yeah, I was too lazy to write my own parser 😉 So kudos to you for doing all the work 👍 |
I couldn't resist running this new JSON parser through the best JSON test suite (created by Nicolas Seriot). Without further ado, here are the results. TL;DR The problematic ones are the 37 🟠 and the 2 🔴.
|
I think @0xced test case makes a case for a pluggable system. I think shipping with the current json parser that could be swapped out by someone using something like System.Text.Json or newtonsoft if they need something to handle all the edge cases. Might be worth checking out the two errors in the current parser though |
@0xced Awesome idea! I'll add those tests to our tests as well. @phil-scott-78 I'll fix the parsing errors, add an abstraction for it, and move it to a plugin in the repo. I still think it's worth having a JSON renderer without dependencies. It has turned out to be a very useful feature in Rich. |
A side note; we really want to be as tolerant as possible since we're only renderering the JSON, so only the red and purple tests should be fixed. |
67856c5
to
a5029c8
Compare
@spectreconsole/maintainers This one is ready for review now. |
The two red tests are really here to test for stack overflow, which is exactly what happened. The blue and purple results are not really relevant, they are implementation defined. The orange results are the most interesting: they are all tests of invalid JSON where the parser did not throw any exception. They could definitively be used to make sure the JSON is rendered in another shape/color to indicate that the JSON is invalid. But that could turn out to be even harder than writing a conforming JSON parser that throws on invalid input… And as Patrick said, since the only purpose of this parser is to render the JSON, being very tolerant is a good thing and I don't think it would be worthwhile to design a pluggable parser system. |
Hi @patriksvensson, based on my review comments above, I've done some minor reworking of the JsonText class, see: https:/FrankRay78/spectre.console/tree/json (this commit: 6c1efc1) I feel it's cleaner removing both statics and relocating the number of spaces to indent each time (unless of course you have reasons for doing this which I'm not aware of). Happy to discuss further or defer to your judgement here though. |
@FrankRay78 I'm afraid I don't agree on removing the static shared instance. The builder has no state, so newing up a new instance of the builder really gives no benefit. I also don't think that indenation spaces should be part of styles, which contains |
I've come to hate all static classes because of how they hinder unit testing and obscure dependencies. However, that's personal preference and in this case, JsonText is a self contained module with a covering integration test and not intended for public extension. Regarding IndentationSpaces, I did look at the possibility of having it located on the BuilderContext class, which would allow the styles class to remain unchanged, preserving the current name. Anyhow, these were my PR review thoughts - take (or leave) what you feel is most helpful. Happy to mark the review comments as resolved. |
Ok, probably the final review comment from me... Given the json parser test results above provided by @0xced, I did wonder whether individual test cases to be fixed could/should be included in the VS solution, accompanied with (at least) failing tests written for each, to keep track of known issues to fix - either now or at some point later. eg. for both red tests, that would involve including the following files: https:/nst/JSONTestSuite/blob/master/test_parsing/n_structure_open_array_object.json However, when I opened up each of these json files, it became clear these are very verbose and almost certainly edge case parsing conditions. Also, we'd really need to include all tests in the VS solution, and as such, seems to be a sledgehammer cracking a nut approach. Then I noticed that actual parsers had been uploaded into the JSONTestSuite repo and integrated into the test harness there, used to produce the following html report (located here: https:/nst/JSONTestSuite/tree/master/results) eg: I also noticed that @0xced had previously uploaded both dotnet parsers into the JSONTestSuite repo, and so I wondered, whether for good measure and in a similar way, actually if the Spectre.Console.Json parser should be uploaded alongside the other parsers, and the html report regenerated - as a way to fully document the exact behaviour of the spectre Json parser against all others. We could link out to that report somewhere in the Spectre.Console documentation should end users be interested in this. And over time should the Json parser be updated, then new versions could be uploaded into the JSONTestSuite repo and the html report regenerated. (possibly completely over the top, but I would be remiss not to mention this thought as an outcome of my PR review) |
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.
All my comments have been addressed in Patrik's comment here - #1086 (comment)
This PR adds a new widget
JsonText
which can be used to render syntax highlighted JSON.Resolves #1051