Skip to content
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

Generate service READMEs and fix module descriptions #766

Merged
merged 7 commits into from
Oct 14, 2021
Merged

Generate service READMEs and fix module descriptions #766

merged 7 commits into from
Oct 14, 2021

Conversation

jdisanti
Copy link
Collaborator

@jdisanti jdisanti commented Oct 14, 2021

Description

Testing

  • Unit tested where it made sense
  • Manually inspected generated README.md files and Cargo.toml files

Checklist

  • I have updated CHANGELOG.md
  • I have updated aws/SDK_CHANGELOG.md if applicable

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Comment on lines +35 to +39
writer.raw(
"""
# $moduleName

**Please Note: The SDK is currently released as an alpha and is intended strictly for
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Long term, I think we should probably generate the entire top-level README.md file in aws-sdk-rust so that it's kept in sync with this one. I omitted a few sections in this README since I didn't think all of them made sense in this context.

For now, we will need to remember to update both READMEs, unless you think the investment is worth it now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we can put an HTML comment in both noting that they both need to be updated

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm skeptical anyone would actually see that when updating one of the READMEs unless they were updating the entire README rather than just a part of it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Filed #770 to track this.

AwsService(
service = service.id.toString(),
module = sdkId,
moduleDescription = "AWS SDK for $title",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Examples for this value:

  • AWS SDK for Amazon Simple Storage Service
  • AWS SDK for AWS Identity and Access Management
  • AWS SDK for Amazon DynamoDB

I think I would prefer to it to be "AWS SDK for S3", but I'm not sure if there's a reliable model attribute for that. One would think the Service sdkId would work, but that value doesn't look great for a number of services.

@jdisanti jdisanti requested review from rcoh and Velfi October 14, 2021 00:58
@jdisanti
Copy link
Collaborator Author

jdisanti commented Oct 14, 2021

I'm not sure why these changes would cause the errors CI is seeing for the generated transcribestreaming crate.

Edit: It was caused by the event stream allow list being broken by a rename in build.gradle.kts.

Copy link
Collaborator

@rcoh rcoh left a comment

Choose a reason for hiding this comment

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

LGTM, only major concern is deterministic ordering

Comment on lines +35 to +39
writer.raw(
"""
# $moduleName

**Please Note: The SDK is currently released as an alpha and is intended strictly for
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we can put an HTML comment in both noting that they both need to be updated

fun finalize(
settings: RustSettings,
model: Model,
manifestCustomizations: Map<String, Any?>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

add a comment about what this map is since it's the fairly opaque str->any?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Opted to type alias it and add a large doc comment to the alias.

@@ -38,7 +41,8 @@ class CargoTomlGenerator(
"dev-dependencies" to dependencies.filter { it.scope == DependencyScope.Dev }
.associate { it.name to it.toMap() },
"features" to cargoFeatures.toMap()
)
).deepMergeWith(manifestCustomizations)
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need to do anything to ensure deterministic ordering?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The decorator order should be sufficient for this.

private val modules: List<RustModule>,
private val customizations: List<LibRsCustomization>
) {
fun render(writer: RustWriter) {
writer.first {
customizations.forEach { it.section(LibRsSection.Attributes)(this) }

val libraryDocs = settings.getService(model).getTrait<DocumentationTrait>()?.value ?: settings.moduleName
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd probably lean towards being more explicit here and having LibRsGenerator accept a service, but fine either way. Using the doc trait directly here makes it a little trickier to customize (although I guess we could always do it as a model customization?

I'm just imagining how this will work when we want to start rendering examples in here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can easily add a section (similar to the attributes section above) for rendering examples.

@jdisanti jdisanti merged commit cf46613 into smithy-lang:main Oct 14, 2021
@jdisanti jdisanti mentioned this pull request Oct 14, 2021
3 tasks
@jdisanti jdisanti deleted the crates-io-docs branch October 14, 2021 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generated module description contains HTML
2 participants