feat: generate default generic client implementations #1468
Conversation
lauzadis
left a comment
lauzadis
left a comment
There was a problem hiding this comment.
I don't see anything in this PR that deals with default implementations, did we only have to relocate the protocols for this to work?
| { | ||
| "id": "ece99a81-4736-43df-a20e-9a367e36370b", | ||
| "type": "feature", | ||
| "description": "Started generating default implementations for generic clients" |
There was a problem hiding this comment.
nit: "Started" -> "Start", or just "Generate..."
Use the imperative present tense (e.g., "change" not "changed" nor "changes")
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
| package software.amazon.smithy.kotlin.codegen.aws.protocols.core | ||
| package software.amazon.smithy.kotlin.codegen.protocols.core |
There was a problem hiding this comment.
Why did this and other packages change?
There was a problem hiding this comment.
Because we refactored the protocols core and others from smithy-aws-kotlin-codegen into smithy-kotlin-codegen
There was a problem hiding this comment.
Can you clarify why we need to move the AWS protocols from smithy-aws-kotlin-codegen to smithy-kotlin-codegen? IIRC this was an intentional split to separate AWS protocols from generic Smithy ones (CBOR). Customers could still add a dependency on the smithy-aws-kotlin-codegen module if they want to use an AWS protocol
| import software.amazon.smithy.model.shapes.ServiceShape | ||
| import software.amazon.smithy.model.shapes.ShapeId | ||
| import kotlin.collections.plus | ||
|
|
There was a problem hiding this comment.
Is this import necessary? Same below
There was a problem hiding this comment.
It was added by Intellij, it's not necessary
We also had to refactor/relocate |
| package software.amazon.smithy.kotlin.codegen.aws.protocols.core | ||
|
|
||
| import software.amazon.smithy.codegen.core.Symbol | ||
| import software.amazon.smithy.kotlin.codegen.aws.protocols.json.AwsJsonHttpBindingResolver | ||
| import software.amazon.smithy.kotlin.codegen.core.KotlinWriter | ||
| import software.amazon.smithy.kotlin.codegen.model.expectShape | ||
| import software.amazon.smithy.kotlin.codegen.protocols.core.AwsHttpBindingProtocolGenerator | ||
| import software.amazon.smithy.kotlin.codegen.protocols.json.AwsJsonHttpBindingResolver |
There was a problem hiding this comment.
Correctness: Unit test classes should be in the same package and module as the units they test.
| use aws.protocols#awsJson1_0 | ||
| use aws.protocols#restJson1 | ||
| use smithy.rules#operationContextParams | ||
| use smithy.rules#endpointRuleSet | ||
| use aws.api#service | ||
| @awsJson1_0 | ||
| @restJson1 | ||
| @service(sdkId: "UnionOperationTest") |
There was a problem hiding this comment.
Question: Why did we change the subject protocol for this test?
There was a problem hiding this comment.
With the new code generation changes, the function used in these tests couldn't resolve awsJson1_0 in it's dependencies
There was a problem hiding this comment.
Do we need to update the dependencies somewhere so it can resolve awsJson1_0?
3 participants
Issue #
N/A
Description of changes
Refactored protocols and protocol generator supplier so generic client implementations can be generated
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.