feat: add support for smithy document type #671
Conversation
aajtodd
left a comment
aajtodd
left a comment
There was a problem hiding this comment.
I wish we could improve the ShapeValueGenerator some but the ugliness there predates this PR.
Looks good overall, one suggestion/improvement to look into.
| // TODO - deal with document members | ||
| writer | ||
| .write("val serializer = #T()", RuntimeTypes.Serde.SerdeJson.JsonSerializer) | ||
| .write("serializer.serializeDocument(input.#L)", memberName) |
There was a problem hiding this comment.
I think we can/should make this part of the StructuredData serializer/parser implementations. More than likely they can just be part of the payloadSerializer/Deserializer implementation and then document can just be added to L450 and be handled in the same branch as ShapeType.STRUCTURE, ShapeType.UNION. This will remove need to have any hard coding on JSON here.
In fact because StructSerializer/DeserializerGenerator has support for documents you probably don't even need to do anything special other than add ShapeType.DOCUMENT to the aforementioned branch.
There was a problem hiding this comment.
@ianbotsf Make sure you take a look at the updates for this.
ianbotsf
left a comment
ianbotsf
left a comment
There was a problem hiding this comment.
Comment: I see there's still an unaddressed TODO in SerializeStructGeneratorTest.kt around tests for document type. We should add some tests there or (if they truly aren't necessary) just delete the TODO.
| ) : NodeVisitor<Unit> { | ||
|
|
||
| override fun objectNode(node: ObjectNode) { | ||
| if (currShape.type === ShapeType.DOCUMENT) { |
There was a problem hiding this comment.
Question: Why ===? Won't == be sufficient here?
There was a problem hiding this comment.
You have witnessed firsthand my javascript reflex. I didn't even realize I did this!
| writer | ||
| .writeInline("#T(\n", RuntimeTypes.Core.Smithy.Document) | ||
| .indent() | ||
| .writeInline("listOf(\n") | ||
| .indent() | ||
|
|
||
| node.elements.forEach { | ||
| generator.writeShapeValueInline(writer, currShape, it) | ||
| writer.writeInline(",\n") | ||
| } | ||
|
|
||
| writer | ||
| .dedent() | ||
| .writeInline(")\n") | ||
| .dedent() | ||
| .writeInline(")") |
There was a problem hiding this comment.
Style: The manual indenting/dedenting is unnecessary here:
writer.withBlock("#T(", ")", RuntimeTypes.Core.Smithy.Document) {
withBlock("listOf(", ")") {
node.elements.forEach {
generator.writeShapeValueInline(writer, currShape, it)
writeInline(",\n")
}
}
}There was a problem hiding this comment.
So this was deliberate -- I had writeBlockified at first, but it doesn't generate output in the spirit of the method writeShapeValueInline - keyword being "inline", you end up with some extra line breaks. The code is still functional but not necessarily reader-friendly and the output isn't truly inline at that point.
It doesn't super matter to me, especially considering that this ShapeValueGenerator is effectively only generating for test code, but I suppose it could be otherwise in the future.
We could (probably should) also expose withInlineBlock on the writer, because the manual *denting is indeed nasty. I could see that getting use elsewhere in codegen.
At the very least, if we do keep it, a comment might be warranted.
There was a problem hiding this comment.
Oh I see, I missed that the last write doesn't want a newline. withInlineBlock is a great idea and might lead to a simplification of this new block and probably some existing blocks as well. Up to you if you want to tackle it in this PR. Otherwise, my concern is addressed.
This is a miss on my part. We should have our own tests here, I did in fact add them for the explicit |
…ent serde codegen test
| if (index < node.elements.size - 1) { | ||
| writer.writeInline(",\n") | ||
| } |
There was a problem hiding this comment.
Style: There's no reason to omit the final comma. You could simplify this to the original forEach loop that always adds the trailer.
|
Kudos, SonarCloud Quality Gate passed!
|
3 participants
Issue #
(#123)
Description of changes
Wire in codegen for
Documentserde.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.