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.

Sign up for GitHub

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

Jump to bottom

Improve generated documentation #779

Merged
merged 20 commits into from Oct 20, 2021
Merged

Improve generated documentation #779

merged 20 commits into from Oct 20, 2021

Conversation

jdisanti
Copy link
Collaborator

Description

A lot of generated code was missing documentation. This PR fills in all missing docs for generated SDK crates, and also fills in missing docs on the smithy-types crate that is partially re-exported by SDK crates.

Testing

  • Manually inspected compiled docs.
  • Automated detection of missing docs with a lint.

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.

@jdisanti jdisanti marked this pull request as draft October 19, 2021 00:56
rcoh
rcoh reviewed Oct 19, 2021
View reviewed changes
aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/AwsPresigningDecorator.kt Outdated Show resolved Hide resolved
aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/CredentialProviders.kt Outdated
@@ -77,6 +77,7 @@ class CredentialProviderConfig(runtimeConfig: RuntimeConfig) : ConfigCustomizati
self
}

/// Set the credentials provider for this service
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: on region it's sets but on this it's set

aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/SharedConfigDecorator.kt Outdated
@@ -73,10 +73,11 @@ class NewFromShared(runtimeConfig: RuntimeConfig) : ConfigCustomization() {
ServiceConfig.ConfigImpl -> writable {
rustTemplate(
"""
/// Creates a new service config from a shared config.
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe have this link to the service::Config here? Could name it ${runtimeConfig.moduleName}::Config I think

codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/rustlang/RustModule.kt
Comment on lines -15 to -22
fun default(name: String, public: Boolean): RustModule {
// TODO: figure out how to enable this, but only for real services (protocol tests don't have documentation)
/*val attributes = if (public) {
listOf(Custom("deny(missing_docs)"))
} else {
listOf()
}*/
return RustModule(name, RustMetadata(public = public))
Copy link
Collaborator

Choose a reason for hiding this comment

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

:-) happy to see this gone!

codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/rustlang/RustWriter.kt
Comment on lines -205 to -207
// We need to filter out blank lines—an empty line causes the markdown parser to interpret the subsequent
// docs as a code block because they are indented.
.filter { it.isNotBlank() }
Copy link
Collaborator

Choose a reason for hiding this comment

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

did you verify this was safe to remove?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, we need to vet the codegen diff for this once that's available.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we should review the output of cargoDocs, I remember a lot of docs were broken by this

...kotlin/software/amazon/smithy/rust/codegen/smithy/generators/error/CombinedErrorGenerator.kt
@@ -123,38 +130,46 @@ class CombinedErrorGenerator(
writer.rustBlock("impl ${symbol.name}") {
writer.rustTemplate(
"""
/// Creates a new `${symbol.name}`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

since this is in scope, you can probably just wrap this in [ ]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would make this doc link to itself.

rust-runtime/smithy-types/src/base64.rs Outdated
@@ -81,10 +81,14 @@ pub fn decode<T: AsRef<str>>(input: T) -> Result<Vec<u8>, DecodeError> {
decode_inner(input.as_ref())
}

/// Failure to decode a base64 value.
Copy link
Collaborator

Choose a reason for hiding this comment

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

while we're here

Suggested change
/// Failure to decode a base64 value.
/// Failure to decode a base64 value.
#[non_exhausive]

rust-runtime/smithy-types/src/instant/mod.rs Outdated
@@ -66,6 +104,7 @@ impl Instant {
}
}

/// Parses an `Instant` from a string using the given `format`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Parses an `Instant` from a string using the given `format`.
/// Parses an `Instant` from a string using the given [`format`](Format).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since this is an argument, there will be a link to Format already. I'm following the standard library's lead on this of just naming the arg, but letting the link be in the actual function signature.

rust-runtime/smithy-types/src/lib.rs Outdated Show resolved Hide resolved
rust-runtime/smithy-types/src/lib.rs Outdated Show resolved Hide resolved
rcoh
rcoh reviewed Oct 19, 2021
View reviewed changes
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.

thanks for knocking these out! left some comments on possible doc links.

If possible, it'd be great to use this PR to test out the generated code diff stuff!

@jdisanti jdisanti marked this pull request as ready for review October 20, 2021 00:33
@jdisanti jdisanti requested review from rcoh and Velfi October 20, 2021 00:33
@jdisanti
Copy link
Collaborator Author

Codegen diff of S3: smithy-rs-779-s3-codegen-diff.txt

rcoh
rcoh approved these changes Oct 20, 2021
View reviewed changes
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, lets see the output of #791 re: the newlines

aws/SDK_CHANGELOG.md Outdated Show resolved Hide resolved
aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/AwsEndpointDecorator.kt Show resolved Hide resolved
aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/ServiceConfigDecorator.kt Outdated
)
}
} else {
writable {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

there is also emptySection which may be clearer

aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/ServiceConfigDecorator.kt Outdated
writable {}
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

no newline—is your pre-commit hook enabled?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's enabled, but I wrote some code on another machine that didn't have it enabled. I ran pre-commit on all Kotlin files.

codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/rustlang/RustWriter.kt
Comment on lines -205 to -207
// We need to filter out blank lines—an empty line causes the markdown parser to interpret the subsequent
// docs as a code block because they are indented.
.filter { it.isNotBlank() }
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should review the output of cargoDocs, I remember a lot of docs were broken by this

@jdisanti jdisanti enabled auto-merge (squash) October 20, 2021 18:29
@jdisanti jdisanti merged commit 6c9402b into smithy-lang:main Oct 20, 2021
@jdisanti jdisanti deleted the better-generated-docs branch October 20, 2021 18:36
@jdisanti jdisanti mentioned this pull request Oct 20, 2021
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rcoh rcoh rcoh approved these changes

@Velfi Velfi Awaiting requested review from Velfi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jdisanti @rcoh @Velfi