-
Notifications
You must be signed in to change notification settings - Fork 49
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
feat: basic annotation processing #1399
Conversation
…nto feat-codegen-config
…nto feat-codegen-config
A new generated diff is ready to view.
|
A new generated diff is ready to view.
|
A new generated diff is ready to view.
|
@@ -35,8 +35,7 @@ internal class HighLevelOpsProcessor(environment: SymbolProcessorEnvironment) : | |||
|
|||
logger.info("Scanning low-level DDB client for operations and types") | |||
val operations = getOperations(resolver) | |||
|
|||
val codegenFactory = CodeGeneratorFactory(codeGenerator, logger) | |||
val codegenFactory = CodeGeneratorFactory(codeGenerator, logger) // FIXME Pass dependencies |
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.
FYI: I could not figure out how to get a list of dependencies for the ops code generator, I kept getting null
values when trying to find the file which contains DynamoDbClient
…nto feat-finish-todos
A new generated diff is ready to view.
|
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 pretty good, just a few small things left.
// If the property OR any of its arguments reference the annotated type | ||
(propName == name) || | ||
( | ||
propType.arguments.any { arg -> | ||
val argType = arg.type?.resolve() | ||
val argName = requireNotNull(argType?.declaration?.qualifiedName).asString() | ||
argName == name | ||
} | ||
) |
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.
Nit: Parentheses around propType.arguments.any
call are unnecessary
/** | ||
* Renders a ValueConverter for the [KSType]. | ||
* | ||
* Note: The ValueConverter(s) will be rendered without a newline in order to support deep recursion. | ||
* Callers are responsible for adding a newline after the top-level invocation of this function. | ||
*/ | ||
private fun KSType.renderValueConverter(writer: SchemaRenderer) { | ||
writer.apply { | ||
val ksType = this@renderValueConverter | ||
val type = Type.from(ksType).copy(genericArgs = listOf()) | ||
|
||
when { |
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.
Style: I can see how this works recursively and is better than trying to bundle multiple actual types into a single #T
param but an in-class extension method that takes its own class instance as an argument feels awkward. Could this just be a regular instance method that takes the type as an argument:
private fun renderValueConverter(type: KSType)
// calling code
renderValueConverter(prop.type.resolve())
private fun KSType.renderValueConverter(writer: SchemaRenderer) { | ||
writer.apply { | ||
val ksType = this@renderValueConverter | ||
val type = Type.from(ksType).copy(genericArgs = listOf()) |
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.
Suggestion: I see you're removing the generics from ksType
in order to use an equality check with non-parameterized map/list types from Types.Kotlin.Collections
. That works but it's not obvious at first read why you're stripping the generics. It might be cleaner to define an extension method Type.isGenericFor(other: Type)
which performs an equality check on name, package, and nullability.
if (!listCharArray.zip(other.listCharArray).all { (a, b) -> a.contentEquals(b) }) return false | ||
for (i in listCharArray.indices) { | ||
if (!listCharArray[i].contentEquals(other.listCharArray[i])) return false | ||
} | ||
if (listChar != other.listChar) return false | ||
if (listByte != other.listByte) return false | ||
if (listByteArray.size != other.listByteArray.size) return false | ||
if (!listByteArray.zip(other.listByteArray).all { (a, b) -> a.contentEquals(b) }) return false | ||
for (i in listByteArray.indices) { | ||
if (!listByteArray[i].contentEquals(other.listByteArray[i])) return false | ||
} |
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.
Correctness: These updated List<*Array>
comparisons still miss cases where other.list*Array
is longer than this.list*Array
. I suggest:
if (listCharArray.size != other.listCharArray.size) return false
if (!listCharArray.zip(other.listCharArray).all { (a, b) -> a.contentEquals(b) }) return false
…
if (listByteArray.size != other.listByteArray.size) return false
if (!listByteArray.zip(other.listByteArray).all { (a, b) -> a.contentEquals(b) }) return false
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.
Oh, that's what I had in the beginning. I think I missed it / forgot to mention after your first comment, but I was always checking the size of the lists
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.
Oh wow, my eyes totally skipped past that. I apologize. What you originally had was fine.
public object ItemToValueConverter : ValueConverter<Item> { | ||
override fun convertFrom(to: AttributeValue): Item = to.asM().toItem() | ||
override fun convertTo(from: Item): AttributeValue = AttributeValue.M(from) | ||
} |
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.
Nit: KDocs
Quality Gate failedFailed conditions |
A new generated diff is ready to view.
|
…lin (#1451) * initial poc commit of DynamoDB Mapper (#1232) * add support for Mapper initialization (#1237) * implement mapper pipeline (#1266) * initial implementation of codegen for low-level operations/types (#1357) * initial implementation of secondary index support (#1375) * Create new codegen module and refactor annotation processor to use it (#1382) * feat: add Schema generator Gradle plugin (#1385) * Fix plugin test package * add attribute converters for "standard" values (#1381) * fix: schema generator plugin test module (#1394) * feat: annotation processor codegen configuration (#1392) * feat: add `@DynamoDbIgnore` annotation (#1402) * DDB Mapper filter expressions (runtime components) (#1401) * feat: basic annotation processing (#1399) * add DSL overloads, paginators, and better builder integration for DDB Mapper ops codegen (#1409) * chore: split dynamodb-mapper-codegen into two modules (#1414) * emit DDB_MAPPER business metric (#1426) * feat: setup DynamoDbMapper publication (#1419) * DDB Mapper filter expressions (codegen components) (#1424) * correct docs * mark every HLL/DDBM API experimental (#1428) * fix accidental inclusion of expression attribute members in high-level DynamoDB Mapper requests (#1432) * Upgrade to latest build plugin version * fix: various issues found during testing (#1450) * chore: update Athena changelog notes for 1.3.57 (2024-10-18) release (#1449) * feat: update AWS API models * feat: update AWS service endpoints metadata * chore: release 1.3.60 * chore: bump snapshot version to 1.3.61-SNAPSHOT * feat: initial release of Developer Preview of DynamoDB Mapper for Kotlin * Fix Kotlin gradle-plugin version * fix: ddb mapper tests (#1453) * Bump build plugin version --------- Co-authored-by: Matas <[email protected]> Co-authored-by: aws-sdk-kotlin-ci <[email protected]>
Issue #
Description of changes
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.