-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Make operations correctly model service errors #581
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic looks good! Requesting some Kotlin code updates
...main/kotlin/software/amazon/smithy/swift/codegen/model/FlattenServiceErrorsOntoOperations.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my lone comment; though this would work, I think an alternate approach would be better suited.
smithy-swift-codegen/src/main/kotlin/software/amazon/smithy/swift/codegen/CodegenVisitor.kt
Outdated
Show resolved
Hide resolved
…izers for operation errors are being made.
…or service errors.
...main/kotlin/software/amazon/smithy/swift/codegen/integration/HttpBindingProtocolGenerator.kt
Outdated
Show resolved
Hide resolved
...otlin/software/amazon/smithy/swift/codegen/integration/httpResponse/HttpResponseGenerator.kt
Show resolved
Hide resolved
...main/kotlin/software/amazon/smithy/swift/codegen/integration/HttpBindingProtocolGenerator.kt
Outdated
Show resolved
Hide resolved
...main/kotlin/software/amazon/smithy/swift/codegen/integration/HttpBindingProtocolGenerator.kt
Outdated
Show resolved
Hide resolved
...otlin/software/amazon/smithy/swift/codegen/integration/httpResponse/HttpResponseGenerator.kt
Outdated
Show resolved
Hide resolved
smithy-swift-codegen/src/test/kotlin/mocks/MockHttpResponseBindingErrorGenerator.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Please fix failing CI. If further changes are required re-request review.
fd20b70
to
1147526
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case where there are no modeled errors on the service, we should omit the method for topLevelServiceErrorMembers
and the calls to it.
This is consistent with elsewhere in codegen, where components are omitted where they are an empty set or otherwise serve no function.
bd7198d
to
530ea94
Compare
Issue #
Addresses the Issue #1080
Description of changes
Adds service errors into pre-existing code-generation flow for error initializers and error deserializers
Corresponding PR in
aws-sdk-swift
: awslabs/aws-sdk-swift#1114Scope
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.