Adds support for reading binary Ion 1.1 annotations. #666
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## ion-11-encoding #666 +/- ##
==================================================
Coverage ? 65.01%
Complexity ? 5811
==================================================
Files ? 200
Lines ? 25001
Branches ? 4461
==================================================
Hits ? 16254
Misses ? 7434
Partials ? 1313 ☔ View full report in Codecov by Sentry. |
tgregg
force-pushed
the
ion-11-encoding-annotations
branch
from
6246dbe to
b349cf4
Compare
| annotationSequenceMarker.endIndex = annotationSequenceMarker.startIndex + annotationsLength; | ||
| peekIndex = annotationSequenceMarker.endIndex; | ||
| } else { | ||
| // At this point the value must have at least one more byte for each annotation VarSym (one for lower |
There was a problem hiding this comment.
FlexSym?
| // At this point the value must have at least one more byte for each annotation VarSym (one for lower | |
| // At this point the value must have at least one more byte for each annotation FlexSym (one for lower |
There was a problem hiding this comment.
Thanks, some of this was written when these were still named "VarSym" and clearly I forgot to clean up all the comments.
| markerToSet.startIndex = peekIndex; | ||
| markerToSet.endIndex = peekIndex; | ||
| } else if (nextByte != OpCodes.DELIMITED_END_MARKER) { | ||
| throw new IonException("FlexSyms may only wrap symbol zero, empty string, or delimited end."); |
There was a problem hiding this comment.
| throw new IonException("FlexSyms may only wrap symbol zero, empty string, or delimited end."); | |
| throw new IonException("FlexSym 0 may only precede symbol zero, empty string, or delimited end."); |
| markerToSet.startIndex = peekIndex; | ||
| markerToSet.endIndex = peekIndex; | ||
| } else if (nextByte != OpCodes.DELIMITED_END_MARKER) { | ||
| throw new IonException("VarSyms may only wrap symbol zero, empty string, or delimited end."); |
There was a problem hiding this comment.
| throw new IonException("VarSyms may only wrap symbol zero, empty string, or delimited end."); | |
| throw new IonException("FlexSym 0 may only precede symbol zero, empty string, or delimited end."); |
| "E4 09 50 E5 0F 09 50 E6 07 09 0F 0D 50", // SIDs | ||
| "E7 F9 6E 61 6D 65 50 E8 0F F9 6E 61 6D 65 50 E9 1D F9 6E 61 6D 65 0F F3 69 6D 70 6F 72 74 73 50", // FlexSyms | ||
| "E4 12 00 50 E5 0F 24 00 00 50 E6 0E 00 09 0F 0D 50", // SIDs (multi-byte FlexUInts) | ||
| "E7 F2 FF 6E 61 6D 65 50 E8 3C 00 00 F9 6E 61 6D 65 50 E9 F8 00 00 00 F9 6E 61 6D 65 0F E6 FF 69 6D 70 6F 72 74 73 50", // Multi-byte FlexSyms |
There was a problem hiding this comment.
Can you put each test case in a separate line? E.g.
"E4 09 50",
"E5 0F 09 50",
"E6 07 09 0F 0D 50",
There was a problem hiding this comment.
If I do that, I can't reuse the same assertions for all of the parameters. What I can do is use string concatenation to put individual values of each test case on separate lines.
There was a problem hiding this comment.
Oh, I didn't realize that these were all supposed to be the same data encoded in different ways.
| "E7 01 80 50 E7 01 90 50 E8 01 80 01 90 50 E9 09 01 90 01 80 50", | ||
| "E7 02 00 80 50 E7 04 00 00 90 50 E8 08 00 00 00 80 02 00 90 50 E9 90 00 00 00 00 01 90 01 80 50" |
There was a problem hiding this comment.
Can you please put each test case on a separate csv line so that they are run separately? It's easier to read and it's easier to debug a failure because the different test cases get run in isolation from each other.
There was a problem hiding this comment.
See my comment on your related comment. Structuring them this way reduces the number of distinct test methods we need, which I like. I can reformat the CSV lines to be more readable.
Description of changes:
This required adding FlexSym support, which is why this PR is bigger than the title might suggest. You'll notice there are some methods that look similar to others, and may be candidates for DRYing up. I'd like to leave that for a future pass where we can set aside the time to analyze the potential performance impacts.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.