Skip to content

Conversation

@0marperez
Copy link
Contributor

Issue #

N/A

Description of changes

Module documentation is now handled by an integration. It handles links to code examples and hand written documentation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@0marperez 0marperez added the no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly. label Mar 12, 2025
@github-actions
Copy link

A new generated diff is ready to view.

@github-actions

This comment has been minimized.

@github-actions
Copy link

A new generated diff is ready to view.

@github-actions

This comment has been minimized.

@0marperez 0marperez marked this pull request as ready for review March 12, 2025 21:27
@0marperez 0marperez requested a review from a team as a code owner March 12, 2025 21:27
import software.amazon.smithy.model.traits.TitleTrait

/**
* Maps a services SKD ID to its code examples
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit/grammar: service's
typo: SDK

)

/**
* Maps a services SKD ID to its handwritten module documentation file in the `resources` dir.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"service's SDK"

}

private fun generateBoilerPlate(ctx: CodegenContext) = buildString {
// Title must me "Module" followed by the exact module name or dokka won't render it
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: "me" -> "be"


private fun generateCodeExamplesDocs(sdkId: String) = buildString {
appendLine("## Code Examples")
appendLine("To see full code examples, see the $sdkId examples in the AWS Code Library. See ${codeExamples[sdkId]}")
Copy link
Member

@lauzadis lauzadis Mar 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"AWS Code Library" -> "AWS code example library"
https://docs.aws.amazon.com/code-library/latest/ug/what-is-code-library.html

private fun generateHandWrittenDocs(sdkId: String): String = object {}
.javaClass
.classLoader
.getResourceAsStream("aws/sdk/kotlin/codegen/moduledocumentation/${handWritten[sdkId]}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't think we'd need to make a new directory for this, can't we keep the handwritten service docs in /services/?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can keep the handwritten docs in the service dir. It's a little awkward to have handwritten and code generated documentation combined into the same file in my opinion.

Codegen changes would modify the handwritten API.md and the changes would be committed, etc. It seems cleaner to code generate everything.

Copy link
Member

@lauzadis lauzadis Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean keeping them in /services/ but with a different file name. Renaming services/s3/API.md to services/s3/s3.md

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or some other consistent name, as long as it's not API.md, because you're right, that will cause a clash

.getResourceAsStream("aws/sdk/kotlin/codegen/moduledocumentation/${handWritten[sdkId]}")
?.bufferedReader()
?.readText()
?: throw Exception("Unable to read from file ${handWritten[sdkId]}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throw a more specific exception here

/**
* Maps a services SKD ID to its code examples
*/
private val currentCodeExamplesServices = mapOf(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naming: top-level values should be in SCREAMING_SNAKE_CASE. Also "current" is implied.

Something like CODE_EXAMPLES_SERVICES_MAP

* Maps a services SKD ID to its handwritten module documentation file in the `resources` dir.
* The module documentation files MUST be markdown files.
*/
private val currentHandWrittenServices = mapOf(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here. "current" is implied. HAND_WRITTEN_SERVICES_MAP

Comment on lines 86 to 90
if (handWritten.keys.contains(sdkId)) {
append(
generateHandWrittenDocs(sdkId),
)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want our hand-written content to appear above links to the code examples.


private fun generateCodeExamplesDocs(sdkId: String) = buildString {
appendLine("## Code Examples")
appendLine("To see full code examples, see the $sdkId examples in the AWS Code Library. See ${codeExamples[sdkId]}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"see the $sdkId examples"

I think we should be using the value from the TitleTrait rather than the sdkId

TitleTrait: Provides a human-readable proper noun title to services and resources.

@github-actions
Copy link

A new generated diff is ready to view.

@github-actions

This comment has been minimized.

Comment on lines 41 to 47
/**
* Maps a service's SDK ID to its handwritten module documentation file in the `resources` dir.
* The module documentation files MUST be markdown files.
*/
private val HAND_WRITTEN_SERVICES_MAP = mapOf(
"S3" to "S3.md",
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have a consistent naming pattern, I don't think we need to manually maintain this list? Can we just check if the .md file exists and render it?

@github-actions
Copy link

A new generated diff is ready to view.

@github-actions

This comment has been minimized.

@sonarqubecloud
Copy link

@github-actions
Copy link

A new generated diff is ready to view.

generateBoilerPlate(ctx),
)
if (handWritten.keys.contains(sdkId)) {
val handWrittenDocsFile = handWrittenDocsFile(ctx.settings)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naming: the function and the value have the same name, handWrittenDocsFile, which can be hard to read. Consider renaming function to getHandWrittenDocsFile

Also nit/style: you can check if the file exists inside getHandWrittenDocsFile and return null if it doesn't.

That would simplify this code section to just: getHandWrittenDocsFile(ctx)?.let { append(it.readText()) }

@github-actions
Copy link

Affected Artifacts

Changed in size
Artifact Pull Request (bytes) Latest Release (bytes) Delta (bytes) Delta (percentage)
mturk-jvm.jar 1,216,986 1,216,980 6 0.00%
mturk-jvm.jar closure 9,092,954 9,092,946 8 0.00%
ec2-jvm.jar closure 37,256,421 37,256,749 -328 -0.00%
ec2-jvm.jar 29,380,453 29,380,783 -330 -0.00%
s3control-jvm.jar closure 11,937,077 11,937,601 -524 -0.00%
amplify-jvm.jar closure 9,158,255 9,159,076 -821 -0.01%
codebuild-jvm.jar closure 10,402,943 10,404,065 -1,122 -0.01%
s3control-jvm.jar 4,061,109 4,061,635 -526 -0.01%
acmpca-jvm.jar closure 9,049,377 9,051,500 -2,123 -0.02%
codebuild-jvm.jar 2,526,975 2,528,099 -1,124 -0.04%
amplify-jvm.jar 1,282,287 1,283,110 -823 -0.06%
datazone-jvm.jar closure 15,518,070 15,528,283 -10,213 -0.07%
datazone-jvm.jar 7,642,102 7,652,317 -10,215 -0.13%
acmpca-jvm.jar 1,173,409 1,175,534 -2,125 -0.18%
ivsrealtime-jvm.jar closure 9,378,513 9,402,474 -23,961 -0.25%
mediapackagev2-jvm.jar closure 9,325,693 9,370,925 -45,232 -0.48%
dynamodb-jvm.jar closure 11,310,623 11,379,199 -68,576 -0.60%
ivsrealtime-jvm.jar 1,502,545 1,526,508 -23,963 -1.57%
dynamodb-jvm.jar 3,434,655 3,503,233 -68,578 -1.96%
mediapackagev2-jvm.jar 1,449,725 1,494,959 -45,234 -3.03%

@0marperez 0marperez merged commit 4c54b2a into main Mar 14, 2025
19 checks passed
@0marperez 0marperez deleted the api-ref-docs-examples branch March 14, 2025 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants