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

Make module docs customizable and update mod docs for crate reorg #2418

Merged
merged 2 commits into from
Mar 9, 2023

Conversation

jdisanti
Copy link
Collaborator

Motivation and Context

This is needed to correct the module docs behind the enableNewCrateOrganizationScheme feature flag, although it can also be used to allow for codegen decorators to change or augment module documentation (e.g., maybe the AWS SDK adds additional examples/links/etc), and for the service model and codegen config to be incorporated into those docs.


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 force-pushed the jdisanti-reorganize-doc-fixes branch 3 times, most recently from ce42828 to 9b63b2f Compare March 1, 2023 01:23
@github-actions
Copy link

github-actions bot commented Mar 1, 2023

@jdisanti jdisanti marked this pull request as ready for review March 1, 2023 01:48
@jdisanti jdisanti requested review from a team as code owners March 1, 2023 01:48
@jdisanti jdisanti force-pushed the jdisanti-reorganize-doc-fixes branch from 9b63b2f to 241771d Compare March 3, 2023 18:25
@github-actions
Copy link

github-actions bot commented Mar 3, 2023

override fun docs(module: RustModule.LeafModule): String? = when (module) {
ServerRustModule.Error -> "All error types that operations can return. Documentation on these types is copied from the model."
ServerRustModule.Operation -> "All operations that this crate can perform."
// TODO(ServerTeam): Document this module (I don't have context)
Copy link
Contributor

Choose a reason for hiding this comment

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

This module has a decent amount of documentation. We render it in ServerOperationShapeGenerator.kt via a writer.rustTemplate. This is similar to our root docs found in ServerServiceGenerator.kt.

For this reason I think perhaps the docs method should return a Writable? and the dependencies templated in it should be added to the dev-dependencies rather than the normal dependencies.

Copy link
Collaborator Author

@jdisanti jdisanti Mar 8, 2023

Choose a reason for hiding this comment

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

Was actually just refactoring it to be that way in #2432 in order to fix CI. Can we ship this and have that refactor be taken care of there?

Copy link
Contributor

@hlbarber hlbarber Mar 8, 2023

Choose a reason for hiding this comment

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

One additional thing to note during doc generation refactors is that I think it'd be nicer to generate the docs in the actual module rather than on the mod statement for the following reasons:

  • Huge docs won't crowd their parent module
  • They would be more discoverable for developers
  • Intra-doc links would be easier

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I definitely agree that would be better. Given how our module writing system currently works, though, this will require a significant refactor since it will require buffering and separating writables to output into different logical sections. Definitely something to strive towards in the long term, but will require a considerable amount of work.

@jdisanti jdisanti enabled auto-merge March 9, 2023 18:48
@github-actions
Copy link

github-actions bot commented Mar 9, 2023

@jdisanti jdisanti added this pull request to the merge queue Mar 9, 2023
Merged via the queue into main with commit 51df475 Mar 9, 2023
@jdisanti jdisanti deleted the jdisanti-reorganize-doc-fixes branch March 9, 2023 19:35
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.

3 participants