feat: Adds shape visitors for local service #672
Conversation
.../software/amazon/polymorph/smithygo/localservice/shapevisitor/DafnyToSmithyShapeVisitor.java
Show resolved
Hide resolved
.../software/amazon/polymorph/smithygo/localservice/shapevisitor/DafnyToSmithyShapeVisitor.java
Show resolved
Hide resolved
.../software/amazon/polymorph/smithygo/localservice/shapevisitor/DafnyToSmithyShapeVisitor.java
Show resolved
Hide resolved
.../software/amazon/polymorph/smithygo/localservice/shapevisitor/DafnyToSmithyShapeVisitor.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public String timestampShape(final TimestampShape shape) { | ||
| // TODO: Figure out timestamp types when working on timestampShape |
There was a problem hiding this comment.
I would prefer to raise an exception at codegen time if this is encountered -- safer to make codegen fail on unhandled shapes
There was a problem hiding this comment.
AFAIK throwing exception fails the codegen on DDBv2 (new model specifically for Go) shapes, hence the nil part.
There was a problem hiding this comment.
Is DafnyToSmithyShapeVisitor used for both LocalService and AWS SDK shapes?
(Python only implements TimestampShape for AWS SDKs. It raises an exception at codegen time for LocalServices.)
There was a problem hiding this comment.
Ahh I should be able to do that. I'll fix it up in a follow up PR since it's going to be again about ShapeVisitors.
.../software/amazon/polymorph/smithygo/localservice/shapevisitor/DafnyToSmithyShapeVisitor.java
Show resolved
Hide resolved
.../software/amazon/polymorph/smithygo/localservice/shapevisitor/DafnyToSmithyShapeVisitor.java
Show resolved
Hide resolved
There was a problem hiding this comment.
The more I digest this the more I like it.
If you could do a once-over to add some single-line comments inside some of the longer functions to help explain what's going on, that would help others understand it more quickly.
I would really like to see some generated code in context before I approve this but it seems reasonable.
| } | ||
|
|
||
| @Override | ||
| public String timestampShape(final TimestampShape shape) { |
There was a problem hiding this comment.
Ditto as in the reverse conversion -- raise exception at codegen time here?
There was a problem hiding this comment.
Same issue here with the exception.
There was a problem hiding this comment.
Do we need Timestamps in LocalService?
There was a problem hiding this comment.
Responded above, will follow up in the next PR.
...in/java/software/amazon/polymorph/smithygo/localservice/shapevisitor/ShapeVisitorHelper.java
Show resolved
Hide resolved
.../software/amazon/polymorph/smithygo/localservice/shapevisitor/SmithyToDafnyShapeVisitor.java
Show resolved
Hide resolved
Added some comments, renamed some variables to make it self explanatory, and did some better formatting to ease readability - our java formatter is really aggressive and breaks line more than what is generally done in Java. This hinders readability and makes method longer than what it actually should be. Avoided adding lots of comments since it again gets stale and pollutes code (I favor self explanatory code than comments). Let me know if you think we can do few things better. Here's an example of the generated code: https://github.com/aws/aws-cryptographic-material-providers-library/blob/Golang/dev/AwsCryptographyPrimitives/runtimes/go/ImplementationFromDafny-go/awscryptographyprimitivessmithygenerated/to_native.go#L67 |
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.