fix!: improve case utils for more optimal member names #975
Conversation
aajtodd
force-pushed
the
fix-case-utils
branch
from
65c3f5f to
8b42d62
Compare
| // all non-alphanumeric characters: "acm-success"-> "acm success" | ||
| result = result.replace(Regex("[^A-Za-z0-9+]"), " ") | ||
| // emit the current word and update from the next character | ||
| val emit = { next: Char -> |
There was a problem hiding this comment.
this lambda function definitely seems Rusty
| @Test | ||
| fun testAllNames() { | ||
| // Set this to true to write a new test expectation file | ||
| val publishUpdate = true |
There was a problem hiding this comment.
Should we just always have this enabled?
| val publishUpdate = true | ||
| val allNames = this::class.java.getResource("/all-names-test-output.txt")?.readText()!! | ||
| val errors = mutableListOf<String>() | ||
| val output = StringBuilder() |
There was a problem hiding this comment.
can we add a header to the file input, actual?
| fun testClientNames() { | ||
| // jq '.. | select(.sdkId?).sdkId' codegen/sdk/aws-models/*.json > /tmp/sdk-ids.txt | ||
| // Set this to true to write a new test expectation file | ||
| val publishUpdate = true |
There was a problem hiding this comment.
same comment about whether this should be configurable or not
| val publishUpdate = true | ||
| val allNames = this::class.java.getResource("/sdk-ids-test-output.txt")?.readText()!! | ||
| val errors = mutableListOf<String>() | ||
| val output = StringBuilder() |
There was a problem hiding this comment.
also same comment about file header, it makes it easier to know what's what
There was a problem hiding this comment.
Is there a way to make this auto-generated also?
| // with minor changes (s3 and iot as whole words). Previously we used the Java v2 implementation | ||
| // https://github.com/aws/aws-sdk-java-v2/blob/2.20.162/utils/src/main/java/software/amazon/awssdk/utils/internal/CodegenNamingUtils.java#L36 | ||
| // but this has some edge cases it doesn't handle well | ||
| val out = mutableListOf<String>() |
There was a problem hiding this comment.
Style: Could be replaced with return buildList { ... }.
| } | ||
|
|
||
| // Skip cases like `AR[N]s`, `AC[L]s` but not `IAM[U]ser` | ||
| if (peek == 's' && (doublePeek == null || !doublePeek.isLowerCase())) { |
There was a problem hiding this comment.
Nit: (doublePeek == null || !doublePeek.isLowerCase()) → doublePeek?.isLowerCase() != true
| // Skip cases like `DynamoD[B]v2` | ||
| return !(peek == 'v' && doublePeek?.isDigit() == true) |
There was a problem hiding this comment.
Correctness/Question: Why do we want to skip cases like this? It seems to me that "DynamoDBv2" in this case represents the words ["Dynamo", "DB", "v2"] not ["Dynamo", "DBv2"]. I think "IPv4" and "IPv6" are exceptions because they're actually whole initialisms but I think generally "vn" is the start of a new word. Several test expectations below changed for the worse IMHO (e.g., "TlsV1_2" → "Tlsv1_2", "MsChapV1" → "MsChapv1", etc.).
There was a problem hiding this comment.
Changing this results in:
Member Names
DynamoDBv2Action,dynamoDbv2Action => dynamoDBv2Action (expected dynamoDbv2Action)
dynamoDBv2,dynamoDbv2 => dynamoDBv2 (expected dynamoDbv2)
Enum Names
expected 'MsChapv1' != actual 'MsChaPv1' for input: MS-CHAPv1
expected 'Tlsv1' != actual 'TlSv1' for input: TLSv1
expected 'Tlsv1_2' != actual 'TlSv1_2' for input: TLSv1.2
org.opentest4j.AssertionFailedError: expected 'MsChapv1' != actual 'MsChaPv1' for input: MS-CHAPv1
expected 'Tlsv1' != actual 'TlSv1' for input: TLSv1
expected 'Tlsv1_2' != actual 'TlSv1_2' for input: TLSv1.2
Client Names
SESv2,Sesv2 => SeSv2 (expected Sesv2)
There was a problem hiding this comment.
Right, turns out I didn't fully understand the endOfAcronym function. Since it's working on determining whether current is the end, I think we need to add a new special handling case between // Not an acronym in progress and // We aren't at the next word yet:
if (nextChar == 'v' && peek?.isDigit() == true) {
// Handle cases like `DynamoDB[v]2`
return true
}This yields dynamoDbV2, MsChapV1, TlsV1_2, etc.
Full function code
private fun endOfAcronym(current: String, nextChar: Char, peek: Char?, doublePeek: Char?): Boolean {
if (!current.last().isUpperCase()) {
// Not an acronym in progress
return false
}
if (nextChar == 'v' && peek?.isDigit() == true) {
// Handle cases like `DynamoDB[v]2`
return true
}
if (!nextChar.isUpperCase()) {
// We aren't at the next word yet
return false
}
if (peek?.isLowerCase() != true) {
return false
}
// Skip cases like `AR[N]s`, `AC[L]s` but not `IAM[U]ser`
if (peek == 's' && doublePeek?.isLowerCase() != true) {
return false
}
// Skip cases like `DynamoD[B]v2`
return !(peek == 'v' && doublePeek?.isDigit() == true)
}There was a problem hiding this comment.
Nit: This file appears to specifically be a CSV, not just a TXT. Applies to all-names.txt and sdk-ids-test-output.txt as well (and arguably sdk-ids.txt).
| if (publishUpdate) { | ||
| File("sdk-ids-test-output.txt").writeText(output.toString()) | ||
| } | ||
| if (errors.isNotEmpty()) { | ||
| fail(errors.joinToString("\n")) | ||
| } |
There was a problem hiding this comment.
Correctness/Question: Should we really publish the update if there are errors? Or should the error check (and potential fail) happen first?
There was a problem hiding this comment.
the update doesn't overwrite the file, it ends up generated at the root of the project so you can diff them
| // println("completeWordInProgress: $completeWordInProgress, curr: $currentWord") | ||
|
|
|
Kudos, SonarCloud Quality Gate passed!
|
Issue #
fixes awslabs/aws-sdk-kotlin#1064
Description of changes
Pull in
splitOnWordBoundariesimplementation from Rust to fix awslabs/aws-sdk-kotlin#1064.Adds a current snapshot of all the member names from every AWS model and the current expected value to allow us to see the scope of these changes and in the future ensure we don't break them if we need to handle new edge cases. Same for sdk IDs.
This is a breaking change to how names for pretty much everything are formed.
See this commit for the differences in member and client names.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.