Scientific notation numbers with decimal separators support#39406
Conversation
…Included character validations and added some new number to tests
|
|
| doubles[7] = 1e200; | ||
| doubles[8] = -1e200; | ||
| doubles[9] = 1e-200; | ||
| doubles[10] = -1e-200; |
There was a problem hiding this comment.
While these may be reasonable tests, these aren't testing the code change.
To test the code change you'd need to add a test like
[Theory]
[InlineData("6.022e-23", false)]
[InlineData("6.022e-23", true)]
[InlineData("6.022e23", false)]
[InlineData("6.022e23", true)]
[InlineData("6.022e+23", false)]
[InlineData("6.022e+23", true)]
[InlineData("-6.022e-23", false)]
[InlineData("-6.022e-23", true)]
[InlineData("-6.022e23", false)]
[InlineData("-6.022e23", true)]
[InlineData("-6.022e+23", false)]
[InlineData("-6.022e+23", true)]
public static void WriteNumberDecimalScientific(string value, bool indented)
{
WriteSimpleValue(indented, value);
}In JsonElementWriteTests.cs, around line 39 (between WriteNumberScientific and WriteNumberOverprecise).
There was a problem hiding this comment.
roger that I didn't get it right
There was a problem hiding this comment.
Please remove these extra inputs, as this is an unrelated change. (These numbers were parsed by the compiler, not the JSON writer, they aren't evaluated, just written down via Utf8Formatter... because they can't be "invalid")
| return; | ||
| } | ||
|
|
||
| //The non digit character inside the number |
There was a problem hiding this comment.
@vicrdguez Our comment style is to have a space after the // and before the words. If you feel like this comment adds value, please insert the space.
| throw new ArgumentException( | ||
| SR.RequiredDigitNotFoundEndOfData, | ||
| nameof(utf8FormattedNumber)); | ||
| throw new ArgumentException(SR.RequiredDigitNotFoundEndOfData, nameof(utf8FormattedNumber)); |
There was a problem hiding this comment.
Do the tests hit every one of these throws?
There was a problem hiding this comment.
Currently hitting the throws requires either a bug in this method (how we got here 😄) or a JsonElement having gotten into an invalid/corrupted state. (This method is here to guard against that corruption and to serve as validation for a future addition to the writer where it allows you to write the numeric string directly)
Having written both this method and JsonElement, it's probably most efficient for me to write the tests to get the coverage on these throws... after we get this PR in so we get the broken success scenario fixed.
vicrdguez
left a comment
vicrdguez
left a comment
There was a problem hiding this comment.
Commented fixed are done and added more test cases not only for this change, but for more cases
| // because it doesn't need to deal with "NeedsMoreData", or remembering the format. | ||
| if (utf8FormattedNumber.IsEmpty) | ||
| { | ||
| //ThrowHelper.ThrowArgumentException(nameof(utf8FormattedNumber), utf8FormattedNumber); |
There was a problem hiding this comment.
Please don't add commented out code. If there's a ThrowHelper for this already, use it. If not, just delete the comment and be a simple throw.
| [InlineData("1e400", true)] | ||
| [InlineData("-1e400", false)] | ||
| [InlineData("-1e400", true)] | ||
| public static void WriteNumberTooLargeScientific(string value, bool indented) |
There was a problem hiding this comment.
Please revert the changes to this particular test. The formatting variation is already covered by the above two tests, and these changes have made the comments explaining the value of this test case be awkwardly not adjacent to the value that they're explaining. Also, now there are values instead of a single value.
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Please remove the second of the two blank lines. (Or, if you prefer, the first... not like I can tell :))
|
Fixes #39139 |
|
The JSON tests have already reported in on the Windows x86 Release run (no failures, but 5 other library answers haven't posted yet), and this change is likely going to cause another merge conflict for another JSON change outstanding; so using my judgment to go ahead and merge. |
6 participants
Writing Scientific notation numbers with decimal separators and exponent throwed an exception.
The decimal separator and the exponent separator were condition exclusive in the number validation
Fixes #39139.