Skip to content

feat: update JSON protocol codegen for cleanliness#547

Merged
kstich merged 3 commits intosmithy-codegenfrom
protocol_fixes
Dec 12, 2019
Merged

feat: update JSON protocol codegen for cleanliness#547
kstich merged 3 commits intosmithy-codegenfrom
protocol_fixes

Conversation

@kstich
Copy link
Copy Markdown
Contributor

@kstich kstich commented Dec 10, 2019

This commit includes assorted updates to the protocol generators
to improve reusability, set proper visibility, use an import alias
for SerdeContext, and other minor updates.

It also includes a fix for handling the unknown case when
deserializing a union.


Use Model instead of deprecated ShapeIndex. This commit will require using the latest version of Smithy published locally.


Use better name for document body shape serde gen

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

This commit includes assorted updates to the protocol generators
to improve reusability, set proper visibility, use an import alias
for SerdeContext, and other minor updates.

It also includes a fix for handling the unknown case when
deserializing a union.
@kstich kstich requested a review from mtdowling December 10, 2019 19:18
@aws-sdk-js-automation
Copy link
Copy Markdown

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@AllanZhengYP AllanZhengYP changed the title Update JSON protocol codegen for cleanliness feat: update JSON protocol codegen for cleanliness Dec 10, 2019
@kstich kstich requested a review from mtdowling December 11, 2019 05:19
@aws-sdk-js-automation
Copy link
Copy Markdown

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@Override
protected void generateDocumentShapeSerializers(GenerationContext context, Set<Shape> shapes) {
generateDocumentShapeSerde(context, shapes, new JsonShapeSerVisitor(context));
protected void generateDocumentBodyShapeSer(GenerationContext context, Set<Shape> shapes) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "Ser" is not a descriptive enough name and prefer Serializer

Set<Shape> shapesToDeserialize = new TreeSet<>(shapes);
shapes.forEach(shape -> shapesToDeserialize.addAll(shapeWalker.walkShapes(shape)));
shapesToDeserialize.forEach(shape -> shape.accept(visitor));
protected void generateDocumentBodyShapeDeser(GenerationContext context, Set<Shape> shapes) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deser is slightly better than Ser, but still not ideal.

@Override
protected void generateDocumentShapeSerializers(GenerationContext context, Set<Shape> shapes) {
generateDocumentShapeSerde(context, shapes, new JsonShapeSerVisitor(context));
protected void generateDocumentBodyShapeSer(GenerationContext context, Set<Shape> shapes) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment on Ser

@kstich kstich merged commit f6e0b28 into smithy-codegen Dec 12, 2019
@kstich kstich deleted the protocol_fixes branch December 12, 2019 19:45
@aws-sdk-js-automation
Copy link
Copy Markdown

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@lock
Copy link
Copy Markdown

lock bot commented Dec 19, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants