Conversation
tgregg
left a comment
tgregg
left a comment
There was a problem hiding this comment.
Looks good overall. Some comments that go along with comments on amazon-ion/ion-tests#33 .
| // that is there now, before the call) | ||
| _value_tid = read_type_id(); | ||
| if (_value_tid == PrivateIonConstants.tidNopPad) { | ||
| throwErrorAt("Annotations are not allowed for NOP padding"); |
There was a problem hiding this comment.
Perhaps "NOP padding is not allowed within annotation wrappers."
|
|
||
| import org.junit.runner.RunWith; | ||
| import org.junit.runners.Suite; | ||
| import software.amazon.ion.AnnotationEscapesTest; |
There was a problem hiding this comment.
Let's leave the imports lists as-is, or change them in a separate PR.
| { "good" + File.separator + "item1.10n", "good" + File.separator + "symbols.ion" }; | ||
| private static final String[] FILES_WITH_UNKNOWN_SYMBOL_TEXT = { | ||
| "good" + File.separator + "item1.10n", | ||
| "good" + File.separator + "symbols.ion", |
There was a problem hiding this comment.
I'm curious about this. What revision of good/symbols.ion are you using? I don't see any symbols with unknown text in there. Also note that for the others you added here, you can choose a non-zero symbol ID with known text without having to explicitly include a local symbol table. Just choose a symbol from 1-9, which occur in the local symbol table. I added a comment to this effect on amazon-ion/ion-tests#33 .
Once that's fixed, these don't need to be added here. Right now it looks like all these tests are being skipped. If they weren't they should fail.
There was a problem hiding this comment.
good/symbols.ion was in GoodIonTest already, I only moved to add the new files. But you are right it doesn't need to be excluded there.
Submitted a PR to fix the NOP ones, amazon-ion/ion-tests#34
| myExpectedException.expectMessage("Annotations are not allowed for NOP padding"); | ||
|
|
||
| IonDatagram datagram = loadTestFile("bad/nopPadWithAnnotations.10n"); | ||
| assertEquals(1, datagram.size()); |
There was a problem hiding this comment.
Hasn't the code under test already failed on the previous line (as expected)? What are the remaining lines testing?
There was a problem hiding this comment.
good point, fixed.
|
Will wait on amazon-ion/ion-tests#34 before submitting the new PR with the changes |
Changes: * Annotation exception msg * Used existing Nop padding files for testing * Removed files from FILES_WITH_UNKNOWN_SYMBOL_TEXT that don't need to be there
|
The imports are still reordered. |
|
Looks like the |
2 participants
Impl as a special TID marker to skip value. Structs are taken care by default as the field:value pair is parsed together and skipped together
ref: #97