Implement minimal JSON Print

[erichkeane]

The JSON archive uses the RapidJSON::PrettyWriter, which inserts
a bunch of extra whitespace. This change adds an option to use
the RapidJSON::Writer, which generates a minimum-length string.

Signed-off-by: Erich Keane erich.keane@verizon.net

[erichkeane]

Note that this pull request is in response to this conversation here: https://groups.google.com/forum/#!forum/cerealcpp

Some Additional Comments:
I definitely see value in having BOTH a Minimal and a Pretty version available, since debugging with PrettyWriter is much nicer, as is potentially storing to a file, so just altering this to use Writer in all cases is a 'bad' change.

I also attempted to do this with templates, but was unable to do it without doing one of the following:
a- Breaking existing usages of the OutputArchive
b- Requiring a different name for the "Minimal" case.

For point a, defining JSONOutputArchive with a template (even with a default parameter) requires declaring a variable using empty brackets:
template
class JSONOutputArchive : ....{};

This version would require declaration like so:
ostringstream os;
JSONOutputArchive<>(os);

If anyone has guidance as to whether a templated solution would be better and how to do that, I would be glad to give it a try!

Thanks,
Erich

[erichkeane]

I'm thinking of trying Idea 2 from above, so I'll likely push it from another branch. My thoughts are to convert the current JSONOutputArchive to a templated version by a different name (JSONOutputArchiveImpl?). Then, create 2 types that inherit directly from this (JSONOutputArchive, MinimalJSONOutputArchive, name suggestions?!) and expose all of its functionality.

I suspect that would be a nice balance between maintaining existing functionality and making this a compile-time construct.

[dkorolev]

Nit: Extra vertical whitespace.

[dkorolev]

$0.02 from my side.

1. LGTM++
2. Would performance take a hit because of virtual methods? Perhaps templating this policy is a cheaper way to go?
3. Is there a test to add this new behavior to?

Thanks,
Dima

[dkorolev]

And thanks a lot for the change, Erich!

[erichkeane]

dkorolev-- Thanks for the comments! The more I think about this, the more I think the templated version is a better idea, I'll work that over tomorrow I think.
As it sits, the run-time behavior is a little troubling. Additionally, the rapidJSON virtual methods are required for proper inheritance, however I think we can do without those as well.

Stay tuned, I might abandon this review and go with another sometime this weekend.

[AzothAmmo]

Just a heads up we may have to drop in a newer version of RapidJSON sooner than we thought (see #144). Not sure how much that will affect this, but chances are any the general structure of any solution will remain the same.

I haven't had a chance to really look into your ideas yet, but will give feedback when I can.

[erichkeane]

Don't worry about touching this too much. I recently discovered your unit-tests and sandbox tests. I also figured out a way to get this done with templates, so I think this solution is worth abandoning for now.

I think the template version will better work with future versions of rapidjson since it doesn't have to edit it as well.

[chrisdonlan]

Was there ever a solution to this?