Allows values to be macro-aware transcoded value-by-value, adds support for creating macro-aware readers from InputStream, and improves testing. #1005
Conversation
…rt for creating macro-aware readers from InputStream, and improves testing.
| @ParameterizedTest(name = "{0},{1}") | ||
| @MethodSource("allCombinations") | ||
| public void multiValuePartialMacroAwareTranscode(InputType inputType, StreamType outputFormat) throws Exception { | ||
| byte[] data = macroInvocationsProduceEncodingDirectivesThatModifyMacroTable(StreamType.BINARY); |
There was a problem hiding this comment.
Depending on macroInvocationsProduceEncodingDirectivesThatModifyMacroTable here adds a lot of specificity to this test but at a remove, which makes the test harder to read/understand. It isn't directly obvious why any of the substringCount values are the number that they are, and determining the correct value from the data generator method is tedious.
If possible it would be great to use smaller test data defined more locally.
| /** | ||
| * Prepares the reader to perform a macro-aware transcode to the given | ||
| * writer. This must be called before calling [transcodeNext], but is not | ||
| * necessary if calling [transcodeAllTo]. | ||
| * @param writer the writer to which the reader's stream will be transcoded. | ||
| */ | ||
| fun prepareTranscodeTo(writer: MacroAwareIonWriter) |
There was a problem hiding this comment.
What makes the prepareTranscodeTo(writer)/transcodeNext() preferable to transcodeTo(writer)? From a glance over it looks like either would work, so I likely missed something.
There was a problem hiding this comment.
It's not a huge difference, but the current factoring allows for the setup (registering the IVM callback) to happen once, rather than before every value that is transcoded.
There was a problem hiding this comment.
That makes sense, I see it now. It's really more of a question of whether you know that the callback has been registered, right? So could that state also be carried implicitly, by virtue of a private method that assumes the callback has already been registered and a public method which does the registration then calls the private method? Or am I misunderstanding how this works?
There was a problem hiding this comment.
Something like that may be possible. We can follow up in #1015. My main goal with this factoring was to avoid repetitive re-setting of the callback. It's not as easy as "is a callback already registered", because a callback is always registered so that the higher-level reader gets notified when IVMs are encountered. Here, we augment that functionality.
Description of changes:
--limitis specified. It also uses it for--ion-flush-period, which requires the writer to be flushed every N values.byte[]andInputStream. We now also parameterize the macro-aware transcoding tests inEncodingDirectiveCompilationTestto cover all combinations of input type (byte[]orInputStream) and output format (binary or text). Once support for macro-aware transcoding from text is added, an additional dimension of parameterization will be added for that.transcodeValueLIteralmethod in the reader makes these tests pass by recursively callingtranscodeNext()for containers, which ensures that any nested e-expressions will be transcoded as e-expressions and not the expanded values. I've left it as a TODO to find a factoring for this that does not include method recursion, as we've been trying to move away from that. I'm not making that a priority since this is intended as an internal API.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.