Add setting precision for json writers and also add decimal places precision type. #752
Conversation
…uiltStreamWriter and for its settings.
include/json/writer.h
Outdated
| @@ -187,6 +190,8 @@ class JSONCPP_DEPRECATED("Use StreamWriterBuilder instead") JSON_API FastWriter | |||
| bool yamlCompatibilityEnabled_; | |||
| bool dropNullPlaceholders_; | |||
| bool omitEndingLineFeed_; | |||
| UInt precision_; | |||
There was a problem hiding this comment.
We cannot change the size of a class in a public header.
Let's concentrate on the newer CharReader and StreamWriter versions. (Everybody hates them because users do not like to type a few extra characters, but those classes be altered as often as we like without impacting binary-compatibility.)
|
If you drop the changes to the deprecated Reader and Writer classes, then I like this change, though it's big enough to deserve a close review. |
|
These changes only for Writer. For this issue we don't need any changes for Reader. |
| // Allocate a buffer that is more than large enough to store the 16 digits of | ||
| // precision requested below. | ||
| char buffer[36]; | ||
| int len = -1; | ||
|
|
||
| char formatString[15]; | ||
| snprintf(formatString, sizeof(formatString), "%%.%ug", precision); | ||
| if (precisionType == PrecisionType::significantDigits) { | ||
| snprintf(formatString, sizeof(formatString), "%%.%ug", precision); |
There was a problem hiding this comment.
Shouldn't this be e instead of g?
There was a problem hiding this comment.
I was thinking about saving old behaviour with g when used the shortest variant for float, because maybe some guys expect old behaviour from code. Maybe it is better to add third type for e ?
There was a problem hiding this comment.
You're right. Preserving the old way by default is a good idea.
I don't have an opinion on a third type.
Let's drop this latest commit. Then we can merge.
There was a problem hiding this comment.
Ok, i've dropped last commit. U can merge when test will be don.
| @@ -1157,6 +1172,14 @@ StreamWriter* StreamWriterBuilder::newStreamWriter() const | |||
| } else { | |||
| throwRuntimeError("commentStyle must be 'All' or 'None'"); | |||
| } | |||
| PrecisionType precisionType(significantDigits); | |||
| if (pt_str == "significant") { | |||
There was a problem hiding this comment.
These names seem long and ambiguous. Maybe exp and noexp are better names. Or something else?
There was a problem hiding this comment.
I used significant word for old-style precision formatting. see #752 (comment). Answer it please. If we delete old-style formatting - then of course it is better change names for exp and noexp.
| @@ -1757,6 +1757,27 @@ JSONTEST_FIXTURE(ValueTest, precision) { | |||
| expected = "0.25634569487374054"; | |||
| result = Json::writeString(b, v); | |||
| JSONTEST_ASSERT_STRING_EQUAL(expected, result); | |||
|
|
|||
| b.settings_["precision"] = 5; | |||
| b.settings_["precisionType"] = "decimal"; | |||
There was a problem hiding this comment.
Maybe floatPrecision and floatFormat?
There was a problem hiding this comment.
okay, but we broke compability for guys who alreafy using "precision" option for their programs, but I don't think there are much of such people.
There was a problem hiding this comment.
Oh, you're right. I forgot about that.
Yes, we need to keep the word "precision". I guess "precisionType" is fine.
This reverts commit 141ee3a.
|
If there are any complaints, we may have to revert to look more closely. But it looks good to me. Thanks! |
In thread of issue #648 was an idea to make precision settable. So, this PR do that and also add decimal places precision setting.