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

docs: bootstrap API reference docs generation #119

Merged
merged 12 commits into from
May 4, 2021
Merged

Conversation

aajtodd
Copy link
Collaborator

@aajtodd aajtodd commented May 3, 2021

Issue #, if available:
closes smithy-lang/smithy-kotlin#267

Description of changes:

I HIGHLY suggest pulling down this branch and looking at the generated output before providing feedback.

Bootstrap generation of the reference API docs using Dokka. There is still A LOT of improvements and surface area to explore with Dokka, including but not limited to:

  • improved code blocks / syntax highliting (Document status of syntax highlighting Kotlin/dokka#823)
  • Grouping runtime and services separately in nav bar (Suggestion: Group Methods or Classes Kotlin/dokka#84)
  • Including the version information somewhere
  • Ensuring that the KMP target information is displayed (e.g. JVM, Native, JS, etc)
  • Custom Markdown pages that give an introduction to the API, getting started, etc that have their own entry in the nav bar (I see no way to do this in the vanilla Dokka plugin but should be doable in our custom plugin...)
  • Getting smithy-kotlin docs merged in (Merge multiproject (composite build) docs Kotlin/dokka#1864)
  • Fixing our processing of the embedded model docs to spit out valid KDoc (currently we just pre-process the docs to strip tags, etc such that the generated documentation results in valid code. We do not however (e.g.) process HTML lists and convert markdown, etc).

NOTE: I did start updating the changelog but it's not finalized or anything.

I'd like to include some version of the API docs with the M1 release. I fully expect to continue iterating on this as we move forward. I would like our docs to be top notch.

You can include markdown pages with a specific module which for instance allows us to include documentation for specific services. I've shown an example of this for S3.

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

Copy link
Contributor

@ianbotsf ianbotsf left a comment

Choose a reason for hiding this comment

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

High-level feedback about the generated docs themselves:

  • In general, I like the overall look. The font choices are clean, the font weights and color contrast make for easy reading, and the whitespace feels balanced.
  • The left pane is always blank. I would expect to see a tree or a package/class list or something over there.
  • The All modules list on the top-level index page only gives a description for S3, not the other services I built or the common modules.
  • The copy link button in the All modules list copies an anchor link rather than a target link. Is that useful? It seems anchor links for section headers (e.g., Giving Feedback, All modules, etc.) would be more useful.
  • The package view seems broken for some services (e.g., aws.sdk.kotlin.services.lambda.model has some weird nesting behaviors going on like tags aren't being closed).
  • We can only view one of Types, Functions, or Properties at a time. It would be much nicer to see them all at the same time.
  • Many classes/members have no accompanying docstrings.
  • The docstrings for some items are poorly formatted. For example, s3/aws.sdk.kotlin.services.s3/S3Client/getObject has no linebreaks, doesn't vary font styles for file paths, and hyperlinks are missing closing tags.
  • Some referenced types are missing links (e.g., s3/aws.sdk.kotlin.services.s3.model/GetObjectResponse's Properties page is missing links for ByteStream and Instant).
  • Favicon only seems to be applied for the top-index page.

Comment on lines +11 to +12
* S3
* NOTE: S3 is a complicated service, this initial release **DOES NOT** have complete support for all S3 features.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Can we note which features this release does not support? Or if it's easier, which features it does support?

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 not sure we can articulate it well yet but we can try. Let's leave review of the CHANGELOG to another PR though. I started mucking with it here when I was testing some things. We'll have a "bump versions" PR before M1.

README.md Outdated
Comment on lines 43 to 45
```
./gradlew -Paws.services=lambda :codegen:sdk:bootstrap
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: ```sh

Comment on lines +17 to +19
tasks.withType<org.jetbrains.kotlin.gradle.tasks.KotlinCompile> {
kotlinOptions.jvmTarget = "1.8"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Can the JVM target version be read from the parent project?

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 could be made into a variable but it honestly doesn't matter, this is more dictated by what works with the DokkaPlugin than choices we make for our own code we publish.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh good point. I mistakenly thought these should be unified.

Comment on lines +18 to +28
val isInternal = when (d) {
is DClass -> d.isInternalSdk()
is DObject -> d.isInternalSdk()
is DTypeAlias -> d.isInternalSdk()
is DFunction -> d.isInternalSdk()
is DProperty -> d.isInternalSdk()
is DEnum -> d.isInternalSdk()
is DEnumEntry -> d.isInternalSdk()
is DTypeParameter -> d.isInternalSdk()
else -> false
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Why separate when branches for each subclass? Do we not want to check for the internal annotation on some subclasses?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well the annotation itself only applies to certain types but also not every subclass of Documentable inherits the properties necessary to check if it has an annotation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that the annotation itself is visible in docs. Is there a reason to keep it?

http://localhost:8000/client-runtime/aws-client-rt/aws-client-rt/aws.sdk.kotlin.runtime/-internal-sdk-api/index.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahh good point we can suppress it. I don't think it matters either way I was just trying to get most of the things people shouldn't be using or care about out of the docs.


```

See [aws.sdk.kotlin.services.s3.model.GetObjectResponse]
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: This link (and at least one other) doesn't go anywhere. Is it expected to yet?

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 should link appropriately in the actual generated HTML

Copy link
Contributor

Choose a reason for hiding this comment

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

So it does!

@aajtodd
Copy link
Collaborator Author

aajtodd commented May 3, 2021

High-level feedback about the generated docs themselves:

* In general, I like the overall look. The font choices are clean, the font weights and color contrast make for easy reading, and the whitespace feels balanced.

All defaults :)

* The left pane is always blank. I would expect to see a tree or a package/class list or something over there.

You might have to show me this, I'm not sure I follow. I see a navigation bar.

* The **All modules** list on the top-level index page only gives a description for S3, not the other services I built or the common modules.

Yeah this is because we have an API.md for that module which is hand-written. Not sure how to improve status quo for this at the moment.

* The copy link button in the **All modules** list copies an anchor link rather than a target link. Is that useful? It seems anchor links for section headers (e.g., **Giving Feedback**, **All modules**, etc.) would be more useful.

Not sure what you mean, this sounds like a bug in Dokka?

* The package view seems broken for some services (e.g., `aws.sdk.kotlin.services.lambda.model` has some weird nesting behaviors going on like tags aren't being closed).

* We can only view one of **Types**, **Functions**, or **Properties** at a time. It would be much nicer to see them all at the same time.

Dokka default, not sure if it's configurable out of the box or not (I did not see a knob for it but could be wrong)

* Many classes/members have no accompanying docstrings.

Runtime or generated? Generated you'd have to look at the model to verify we don't have a codegen bug. Runtime we just need to check we actually documented it.

* The docstrings for some items are poorly formatted. For example, `s3/aws.sdk.kotlin.services.s3/S3Client/getObject` has no linebreaks, doesn't vary font styles for file paths, and hyperlinks are missing closing tags.

Yup. Known issue. See smithy-lang/smithy-kotlin#136

* Some referenced types are missing links (e.g., `s3/aws.sdk.kotlin.services.s3.model/GetObjectResponse`'s **Properties** page is missing links for `ByteStream` and `Instant`).

Yup. Known issue, these are types from smithy-kotlin. It's unclear how to merge documentation across repos.

* Favicon only seems to be applied for the top-index page.

There is an open issue in Dokka github.

@ianbotsf

Copy link
Contributor

@kggilmer kggilmer left a comment

Choose a reason for hiding this comment

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

Excellent

./gradlew --no-daemon --no-parallel dokkaHtmlMultiModule
```

This will output HTML formatted documentation to `build/dokka/htmlMultiModule`
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion

Mention python3 -m http.server here for people wanting to view local content. I looked for a Kotlin equivalent but there is no one liner I could find.

}

dependencies {
dokkaPlugin(project(":dokka-aws"))
Copy link
Contributor

Choose a reason for hiding this comment

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

question

with the name is it your intent that this would be shared outside of the Kotlin SDK?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope just needed a name. I don't expect it to be published.

tasks.withType<org.jetbrains.dokka.gradle.DokkaTaskPartial>().configureEach {
// each module can include their own top-level module documentation
// see https://kotlinlang.org/docs/kotlin-doc.html#module-and-package-documentation
if (project.file("API.md").exists()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

question

Do you see this mainly being used to describe APIs? Perhaps something like docs.md or supplemental-info.md would be more descriptive?

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 do, or rather only included in the generated API reference docs. This is because it has to follow a particular format for KDoc.


val year = java.time.LocalDate.now().year
val pluginConfigMap = mapOf(
"org.jetbrains.dokka.base.DokkaBase" to """
Copy link
Contributor

Choose a reason for hiding this comment

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

comment

Maybe this JSON would be more maintainable as a file rather than embedded here? I see you resolve file paths though, maybe it wouldn't work with relative paths?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If there was a lot of JSON then maybe but this is literally the only 3 things you can configure with this JSON.


2. Unzip the repository somewhere on your local machine

```sh
> unzip aws-sdk-kotlin-0.1.0-M0.zip
> unzip aws-sdk-kotlin-0.2.0-M1.zip
Copy link
Contributor

Choose a reason for hiding this comment

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

nice you've already written the M1 quick start guide!

*/
class AwsDokkaPlugin : DokkaPlugin() {
init {
println("AwsDokkaPlugin loaded!")
Copy link
Contributor

Choose a reason for hiding this comment

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

question

do you still need this or was part of getting things working initially?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Both? So I was going to remove it but I had so many issues figuring out if it was actually being applied I think it's still useful (it gets printed out per/module when loaded).

@@ -0,0 +1,63 @@
# Module s3
Copy link
Contributor

Choose a reason for hiding this comment

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

comment

It's really cool that we can add additional content like this. I wonder if the doc team would be interested in collaboration here, in supplying some of this for us per-service?

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 potentially! This was mostly an example (feel free to critique) but I could see us adding additional docs for particular services, linking to the developer guide, etc.

@aajtodd aajtodd merged commit e14c101 into main May 4, 2021
@aajtodd aajtodd deleted the api-docs-bootstrap branch May 4, 2021 12:48
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.

Generate documentation with alpha releases
3 participants