Ensures macros can be evaluated from both byte array and InputStream sources. #956
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tgregg
commented
Oct 3, 2024
There was a problem hiding this comment.
I attempted to add the changes in this file to #954; I think I forgot to commit the additional changes after the file move was automatically staged by Git.
| @@ -2497,6 +2497,7 @@ private boolean slowReadMacroInvocationHeader(IonTypeID valueTid, Marker markerT | |||
| macroInvocationId = valueTid.macroId; | |||
| } | |||
| setUserMacroInvocationMarker(valueTid, markerToSet, -1); | |||
| setCheckpoint(CheckpointLocation.BEFORE_UNANNOTATED_TYPE_ID); | |||
There was a problem hiding this comment.
This is the fix that allows InputStreams to work.
There was a problem hiding this comment.
More changes related to the move of EncodingDirectiveReader.
There was a problem hiding this comment.
Drive-by change, see #953 (comment)
| public void structAsParameter() throws Exception { | ||
| @ParameterizedTest | ||
| @EnumSource(InputType.class) | ||
| public void structAsParameter(InputType inputType) throws Exception { |
There was a problem hiding this comment.
This is an example of a test that failed for InputType.INPUT_STREAM before the fix in IonCursorBinary.
popematt
approved these changes
Oct 3, 2024
jobarr-amzn
reviewed
Oct 4, 2024
Comment on lines
+13
to
19
| * Reads encoding directives from the given [IonReader]. This performs a similar function to | ||
| * IonReaderContinuableCoreBinary.EncodingDirectiveReader, though that one requires more logic to handle continuable | ||
| * input. The two could be unified at the expense of higher complexity than is needed by the non-continuable text | ||
| * implementation. If the text reader is replaced with a continuable implementation in the future, | ||
| * IonReaderContinuableCoreBinary.EncodingDirectiveReader should be moved to the top level and shared by both readers. | ||
| * If that were to happen, this class would no longer be needed. | ||
| */ |
There was a problem hiding this comment.
This kind of comment is so valuable, thanks!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes:
RC2 focused on byte array inputs. This PR adds testing for InputStream inputs (which are handled slightly differently in some cases by the binary reader), and includes a fix to IonCursorBinary to make the tests pass.
Before this fix, some of the tests would fail because the cursor's checkpoint was not advanced after reading a macro invocation header, causing the header to be read repetitively.
This PR also includes some minor cleanups that somehow didn't make it into the final revision of my previous PR.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.