chore: abstract codegen logic into a plugin #457
Conversation
…cific plugin types
|
A new generated diff is ready to view: __generated-main...__generated-codegen-plugin |
|
A new generated diff is ready to view: __generated-main...__generated-codegen-plugin |
| * - applying smithy-gradle-plugin to the project to generate code | ||
| * - providing a tasks to generate Kotlin sources from their respective smithy models. | ||
| */ | ||
| class CodegenPlugin : Plugin<Project> { |
There was a problem hiding this comment.
suggestion
in the case we develop plugins for other codegen use cases (endpoints? hlls?) it may be better to have a more targeted name, such as SdkCodegenPlugin or something.
There was a problem hiding this comment.
I think only the id matters in practice.
No I mean that's basically it. Making sure that it behaves the same for protocol tests and sdk bootstrap. The SDK bootstrap process should be mostly the same. The biggest difference is that it may try to resolve the list of "discovered services" more often than it did before (e.g. refreshing the gradle model from intellij). There are some |
|
In local testing found no concerns 👍 |
| data class ProtocolTest(val projectionName: String, val serviceShapeId: String, val sdkId: String? = null) { | ||
| val packageName: String | ||
| get() = projectionName.toLowerCase().filter { it.isLetterOrDigit() } | ||
| } |
There was a problem hiding this comment.
Nit: Since the inputs are all immutable, there seems like no reason to define this as a get() property over an initialized property:
val packageName: String = projectionName.toLowerCase().filter { it.isLetterOrDigit() }| override fun equals(other: Any?): Boolean { | ||
| if (this === other) return true | ||
| if (javaClass != other?.javaClass) return false | ||
|
|
||
| other as SmithyKotlinPluginSettings | ||
|
|
||
| if (serviceShapeId != other.serviceShapeId) return false | ||
| if (packageName != other.packageName) return false | ||
| if (packageVersion != other.packageVersion) return false | ||
| if (packageDescription != other.packageDescription) return false | ||
| if (sdkId != other.sdkId) return false | ||
| if (buildSettings != other.buildSettings) return false | ||
|
|
||
| return true | ||
| } | ||
|
|
||
| override fun hashCode(): Int { | ||
| var result = serviceShapeId?.hashCode() ?: 0 | ||
| result = 31 * result + (packageName?.hashCode() ?: 0) | ||
| result = 31 * result + (packageVersion?.hashCode() ?: 0) | ||
| result = 31 * result + (packageDescription?.hashCode() ?: 0) | ||
| result = 31 * result + (sdkId?.hashCode() ?: 0) | ||
| result = 31 * result + (buildSettings?.hashCode() ?: 0) | ||
| return result | ||
| } |
There was a problem hiding this comment.
Question: Is there a reason not to make this and SmithyProjection data classes so that we don't have to hand-maintain equals/hashCode?
There was a problem hiding this comment.
Well if by hand maintain you mean clicking "generate equals()/hashCode()" from Intellij.
Making them data classes would require all the properties that we want to partake in equals()/hash() to be declared in the constructor. These are effectively builder style classes. We could go that route I suppose but it may limit what kind of DSL we can expose.
| internal var buildSettings: SmithyKotlinBuildSettings? = null | ||
| fun buildSettings(configure: SmithyKotlinBuildSettings.() -> Unit) { | ||
| if (buildSettings == null) buildSettings = SmithyKotlinBuildSettings() | ||
| buildSettings!!.apply(configure) | ||
| } |
There was a problem hiding this comment.
Question: This doesn't look thread-safe. Is it certain that buildSettings will only be called in single-threaded contexts?
There was a problem hiding this comment.
As far as I am aware configuration lifecycle for a single project is expected to be single threaded
| internal fun ObjectNode.Builder.withOptionalMember(key: String, member: String?): ObjectNode.Builder = apply { | ||
| if (member == null) return this | ||
| return withMember(key, member) | ||
| } | ||
|
|
||
| internal fun ObjectNode.Builder.withOptionalMember(key: String, member: Boolean?): ObjectNode.Builder = apply { | ||
| if (member == null) return this | ||
| return withMember(key, member) | ||
| } | ||
|
|
||
| internal fun <T : ToNode> ObjectNode.Builder.withOptionalMember(key: String, member: T?): ObjectNode.Builder = | ||
| withOptionalMember(key, Optional.ofNullable(member)) |
There was a problem hiding this comment.
Suggestion: withOptionalMember has two meanings here: Option<T> and T?. Given that the Option<T> meaning comes from Smithy and we can't (easily) change it, I suggest renaming the methods in this project to withNullableMember.
| val projectionHash = project.objects.property(Int::class.java) | ||
| projectionHash.set(0) | ||
| project.codegenExtension.projections.all { | ||
| projectionHash.set(projectionHash.get() + hashCode()) | ||
| } | ||
| inputs.property("projectionHash", projectionHash) |
There was a problem hiding this comment.
Question: This projection hash is order-independent. (That is, A + B ≡ B + A.) That's normally an undesirable quality in hashes but I don't fully understand the use case here. Should order matter for this hash specifically?
There was a problem hiding this comment.
This hash is a hacky way of not creating a proper list property input for the task. It handles ensuring that the up-to-date checks work if the plugin settings change. Defining it as a list property would require that they type implement Serializable
Gradle supports three main categories of inputs and outputs:
Simple values
Things like strings and numbers. More generally, a simple value can have any type that implements Serializable.
Filesystem types
These consist of the standard File class but also derivatives of Gradle’s FileCollection type and anything else that can be passed to either the Project.file(java.lang.Object) method — for single file/directory properties — or the Project.files(java.lang.Object...) method.
Nested values
Custom types that don’t conform to the other two categories but have their own properties that are inputs or outputs. In effect, the task inputs or outputs are nested inside these custom types.
I tried this but it had other drawbacks and was more pervasive of a change. In practice we just care that it changed so a hash of each projection seemed reasonable.
| } | ||
|
|
||
| val codegenConfig = createCodegenConfiguration() | ||
| val buildTask = project.tasks.register<SmithyBuild>(GENERATE_SMITHY_PROJECTIONS_TASK_NAME) { |
There was a problem hiding this comment.
Nit: buildTask is never used.
|
Kudos, SonarCloud Quality Gate passed!
|
|
A new generated diff is ready to view: __generated-main...__generated-codegen-plugin |
3 participants
Issue #
N/A
Description of changes
aws-sdk-kotlinand generate code with the correct versions and task relationships already taken care of for you. All of the inputs and outputs are also (hopefully) correctly defined.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.