Conversation
popematt
left a comment
popematt
left a comment
There was a problem hiding this comment.
To avoid adding more public methods to Timestamp, I propose having all of Timestamp's toString-like methods just use the default.
I agree with this.
Overall looks good, but it would be ideal to have another test case or two for the text writer.
| /** | ||
| * <b>NOT FOR APPLICATION USE. This class may be removed at any time.</b> | ||
| */ | ||
| public final class _Private_Trampoline { |
There was a problem hiding this comment.
FYI, you could fairly trivially rewrite this in Kotlin and mark it as internal. You don't even need to include the class.
E.g. _Private_Trampoline.kt
package com.amazon.ion
/**
* **NOT FOR APPLICATION USE. This method may be removed at any time.**
* Trampoline to the non-public `Timestamp.toString(Int)` method.
*/
internal fun print(timestamp: Timestamp, maximumDigits: Int): String {
return timestamp.toString(maximumDigits)
}| super.testAnnotationNotSetToIvmOnStartOfStream(); | ||
| } | ||
|
|
||
| @Test(expected = IllegalArgumentException.class) |
There was a problem hiding this comment.
We should probably add 1 or 2 test cases to ensure that the max length is respected when changed.
There was a problem hiding this comment.
Or did you mean something else?
There was a problem hiding this comment.
That's exactly it. I guess I missed that because I was expecting the TextWriter tests to be in TextWriterTest.java.
…d digits that are written to text representations.
tgregg
force-pushed
the
timestamp-tostring-limit
branch
from
b747ee8 to
25ff035
Compare
|
Revision 2:
|
2 participants
Description of changes:
The Timestamp class holds fractional precision in a BigDecimal, where in many cases that precision can be compactly conveyed (e.g.
0d-1000000, which is a tuple of(0, 1000000)). In text, all those digits must be written explicitly. To avoid attempting to write an excessive number of digits, this PR proposes a configurable maximum, with the default set arbitrarily to 10,000, which is a preposterous number of digits. If the maximum is exceeded, an error is raised (rather than silently losing precision).To avoid adding more public methods to
Timestamp, I propose having all ofTimestamp'stoString-like methods just use the default. In the unlikely event that a user needs to raise the limit, they can use a text writer to serialize the timestamp. The tradeoff is the need for the new_Private_Trampolineutility class that allows us to call the new non-publicTimestamp.toString(int)variant inIonWriterSystemText(which lies in a different Java package) without exposing it publicly inTimestamp.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.