Implements Ion 1.1 managed writer#1141
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## ion11 #1141 +/- ##
========================================
Coverage ? 69.81%
Complexity ? 6507
========================================
Files ? 211
Lines ? 26337
Branches ? 4560
========================================
Hits ? 18386
Misses ? 6529
Partials ? 1422 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| startMacro(macro) | ||
| writeInt(1) | ||
| endMacro() |
There was a problem hiding this comment.
Is it possible to allow the user to elide the start/end macro calls since they've already declared this list to have that macro shape?
There was a problem hiding this comment.
Maybe, but I'd rather be explicit about things. Having the start and end macro calls means that the managed writer can validate all of the parameters of a macro. Without those calls, for example, if there's a macro that takes 3 tagged args, and you are providing 2 tagged args in each iteration of your content-writing loop, the writer will accept it and write data that you are not intending to write.
| } | ||
|
|
||
| override fun stepOut() { | ||
| // TODO: Make sure you can't use this function to step out of a macro? |
There was a problem hiding this comment.
Just for readability of the code written by users? Would it look better if the had "stepInMacro(...)" that was paired with "stepOut()"?
There was a problem hiding this comment.
- It's also because there's no need to have a generic
stepOut()method. The user/application code should know what sort of container it stepped into because it previously calledstepIn(IonType). While I can't change thestepOut()method, I can avoid introducing new uses. - For the data model containers, it doesn't matter quite as much, but for macros, we could have special logic where we validate that the correct number of parameters were written and/or add
absentArgument()for any trailing optional args that have been omitted.
Now that I've had more time to think about it, I wish I hadn't used stepOut() at all in the Ion 1.1 raw writer. 🤷♂️
src/main/java/com/amazon/ion/impl/IonManagedWriterOptions_1_1.kt
Outdated
Show resolved
Hide resolved
| if (sid >= 0) { | ||
| handleSymbolToken(UNKNOWN_SYMBOL_ID, annotationText, SymbolKind.ANNOTATION, userData) | ||
| } else { | ||
| handleSymbolToken(sid, annotationText, SymbolKind.ANNOTATION, userData) | ||
| } |
There was a problem hiding this comment.
Why UNKNOWN_SYMBOL_ID only when sid is valid? Is the conditional supposed to be reversed here?
There was a problem hiding this comment.
Yeah, this is wrong. There should be no condition here at all because that sort of stuff is handled in handleSymbolToken()
| content ?: throw IonException("Cannot write null.symbol for tagless symbol") | ||
| if (container.childType != OpCode.TE_SYMBOL_FS) { | ||
| val expectedType = container.expectedChildTypeName() | ||
| throw IllegalArgumentException("Cannot write a symbol when a tagless $expectedType is expected.") |
There was a problem hiding this comment.
This check is missing on the other write* methods - eg. writeInt seems to let you write a tagless int no matter what the expected child type of the tagless container is. If those checks should be there it might be worth putting in some TODOs.
There was a problem hiding this comment.
There is a blanket TODO for that at the top of the class.
3 participants
Issue #, if available:
Description of changes:
IonManagedWriter_1_1and testsBufferedAppendableFastAppendableso that we can buffer even when there's a text writer.IonWriterBuilderof some sort.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.