-
Notifications
You must be signed in to change notification settings - Fork 24
Add sphinx doc generation #418
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
Conversation
.../core/src/main/java/software/amazon/smithy/python/codegen/generators/StructureGenerator.java
Outdated
Show resolved
Hide resolved
a0c24c0 to
0036e2e
Compare
codegen/aws/core/src/main/java/software/amazon/smithy/python/aws/codegen/AwsDocConverter.java
Outdated
Show resolved
Hide resolved
codegen/core/src/main/java/software/amazon/smithy/python/codegen/writer/PythonWriter.java
Show resolved
Hide resolved
codegen/core/src/main/java/software/amazon/smithy/python/codegen/generators/UnionGenerator.java
Show resolved
Hide resolved
codegen/core/src/main/java/software/amazon/smithy/python/codegen/generators/SetupGenerator.java
Outdated
Show resolved
Hide resolved
codegen/core/src/main/java/software/amazon/smithy/python/codegen/writer/PythonWriter.java
Show resolved
Hide resolved
| // If no space found, force wrap at maxLineLength | ||
| wrapAt = maxLineLength; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this work for things like links and code that may stretch beyond the limit. Is this going to create rendering issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does create some; with the current docs there is only one link I can find with this issue.
What is the ideal behavior here? Should we allow these docs to exceed the max line length?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's a link, probably. That only affects rendering in the code. There's a question of if we even want the links in the docstrings, but I don't think we need to solve that now.
764264e to
f1b9ced
Compare
Draft commit - whitespace changes still need to be addressed
f1b9ced to
1a36552
Compare
codegen/aws/core/build.gradle.kts
Outdated
| dependencies { | ||
| implementation(project(":core")) | ||
| implementation(libs.smithy.aws.traits) | ||
| implementation("org.jsoup:jsoup:1.19.1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | ||
|
|
||
| private static class RstNodeVisitor implements NodeVisitor { | ||
| private final StringBuilder sb = new StringBuilder(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'll probably be better to use a SimpleCodeWriter for this. There's a few major benefits. For starters, it will make indentation management easier. Right now any list values that happen to have a newline will break the indentation. Same for anything in any sort of block.
A secondary benefit is that you can then create a named section that can be pushed in the opening tag and popped in the closing tag. That'll let you intercept those sections later down the line if you need to do customizations. It'll also let you support ordered lists since you can put context into the writer to say which list element you're on. Since context is scoped to sections, you won't have to worry about overriding some parent.
Given time constraints we have right now, this can be a fast follow.
| DocumentationTrait docTrait = shape.getTrait(DocumentationTrait.class).get(); | ||
| String html = docTrait.getValue(); | ||
| String rst = convertHtmlToRst(html); | ||
| DocumentationTrait newDocTrait = new DocumentationTrait(rst); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather run this in PythonWrtier where we write documentation. The format of the documentation trait is commonmark, so putting RST in there isn't really correct.
Speaking of commonmark, you need to account for that. The easiest way is to just render the whole thing to HTML like this. commonmark can contain HTML (and most of the docs we get from AWS are just HTML) but there's more to it.
| return String.format("%s%n%s%n%n", title, "=".repeat(title.length())); | ||
| } | ||
|
|
||
| private static final class OperationGenerationInterceptor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the advantage of doing this over just autoclassing the client itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't need to be AWS specific, nor does the rst doc file generator
| /** | ||
| * Write the files required for sphinx doc generation | ||
| */ | ||
| private static void writeDocsSkeleton( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to do this now, but lets add a todo to make this configurable. We should be able to turn off generating all the sphinx setup files and select a theme.
| import sys | ||
| sys.path.insert(0, os.path.abspath('..')) | ||
| project = 'AWS SDK for Python' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should reference the service title, falling back to the shape name. You could preprocess the model in the AWS side to set the title trait to be based on the SDK ID if you like.
codegen/core/src/main/java/software/amazon/smithy/python/codegen/generators/UnionGenerator.java
Show resolved
Hide resolved
…en/generators/SetupGenerator.java Co-authored-by: Nate Prewitt <nate.prewitt@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once this and that bug are fixed I'm approving
This PR adds generation of sphinx docs. There are a number of changes that are necessary to accomplish this:
AwsDocConverter.java)PythonWriter.java)SetupGenerator.java)*Generator.javafiles as well as the*Section.javafiles)This is still in draft mode pending the following action items:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.