-
Notifications
You must be signed in to change notification settings - Fork 190
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
Codegenerate StructureShape, BlobShape, application, server and Python runtime #1403
Conversation
Generate errors and implement error conversion. Signed-off-by: Bigo <[email protected]>
Signed-off-by: Bigo <[email protected]>
Signed-off-by: Bigo <[email protected]>
A new generated diff is ready to view.
A new doc preview is ready to view. Rust Wrk benchmark report:Duration: 90 sec, Connections: 32, Threads: 2
|
Signed-off-by: Bigo <[email protected]>
A new generated diff is ready to view.
A new doc preview is ready to view. Rust Wrk benchmark report:Duration: 90 sec, Connections: 32, Threads: 2
|
A new generated diff is ready to view.
A new doc preview is ready to view. Rust Wrk benchmark report:Duration: 90 sec, Connections: 32, Threads: 2
|
A new generated diff is ready to view.
A new doc preview is ready to view. Rust Wrk benchmark report:Duration: 90 sec, Connections: 32, Threads: 2
|
A new generated diff is ready to view.
A new doc preview is ready to view. Rust Wrk benchmark report:Duration: 90 sec, Connections: 32, Threads: 2
|
A new generated diff is ready to view.
A new doc preview is ready to view. Rust Wrk benchmark report:Duration: 90 sec, Connections: 32, Threads: 2
|
A new generated diff is ready to view.
A new doc preview is ready to view. Rust Wrk benchmark report:Duration: 90 sec, Connections: 32, Threads: 2
|
Add decorator to add `use aws_smithy_http_server_python::types` to lib.rs.
A new generated diff is ready to view.
A new doc preview is ready to view. Rust Wrk benchmark report:Duration: 90 sec, Connections: 32, Threads: 2
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
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.
2nd pass. Much better regarding code duplication. We should invest into reducing the remaining bits of duplication via decorators, CodegenTarget
, or more inheritance in future PRs.
...kotlin/software/amazon/smithy/rust/codegen/server/python/smithy/PythonCodegenServerPlugin.kt
Outdated
Show resolved
Hide resolved
@@ -61,6 +60,9 @@ class RustCodegenServerPlugin : SmithyBuildPlugin { | |||
symbolVisitorConfig: SymbolVisitorConfig = DefaultConfig | |||
) = | |||
SymbolVisitor(model, serviceShape = serviceShape, config = symbolVisitorConfig) | |||
// Rename a set of symbols that do not implement `PyClass` and have been wrapped in | |||
// `aws_smithy_http_server_python::types`. | |||
.let { PythonServerSymbolProvider(it) } |
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'll also need a symbol provider that renames reserved Python keywords?
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 don't think so. We are not generating any pure python code, everything is shared through the CFFI layer, hence I don't see anything we should really rename here.
class PythonServerCodegenVisitor(context: PluginContext, private val codegenDecorator: RustCodegenDecorator) : | ||
ServerCodegenVisitor(context, codegenDecorator) { | ||
|
||
init { |
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 could have a function that given a symbol provider, sets up the rest of the stuff.
...are/amazon/smithy/rust/codegen/server/python/smithy/generators/PythonApplicationGenerator.kt
Outdated
Show resolved
Hide resolved
import software.amazon.smithy.rust.codegen.server.python.smithy.PythonServerCargoDependency | ||
|
||
/** | ||
* This module contains utilities to render PyO3 attributes. |
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.
Are these extension methods really necessary? It think you get the same syntax sugar by using the Attribute
class from RustTypes
and calling render
.
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.
You are probably right, but I think having these methods in their own separate file is actually nice and I think this file will grow over time with new helpers.. If don't have strong opinions on this, if you feel we should get rid of them I can try to just use the Attribute
class.
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 have tried to use the Attribute
class to render pyo3
attributes, but it messes up the code by removing the //
of any doc block that is rendered just after it and I don't really understand why.
I will add an issue to this block of code to understand what is going on and try to move to the Attribute
class at a later stage.
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 looks like the issue comes from https://github.com/awslabs/smithy-rs/blob/15e1b3425f56ff2606121e99dff11de38424c744/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/rustlang/RustTypes.kt#L377
This line uses writer.raw()
. If I change the line from writer.raw("#$bang[$annotation]")
to writer.rust("##$bang[$annotation]"
it start rendering as I would expect.
I am not sure of the implications of making this change..
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.
...amazon/smithy/rust/codegen/server/python/smithy/generators/PythonServerStructureGenerator.kt
Outdated
Show resolved
Hide resolved
...amazon/smithy/rust/codegen/server/python/smithy/generators/PythonServerStructureGenerator.kt
Outdated
Show resolved
Hide resolved
"SmithyPython" to PythonServerCargoDependency.SmithyHttpServerPython(codegenContext.runtimeConfig).asType() | ||
) | ||
|
||
override fun renderStructure() { |
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 think tacking on attributes is best accomplished through decorators. If the structures we're generating are fundamentally different / additional, then inheritance, CodegenTarget
or splitting are better approaches.
...amazon/smithy/rust/codegen/server/python/smithy/generators/PythonServerStructureGenerator.kt
Show resolved
Hide resolved
/// Custom error. | ||
#[error("{0}")] | ||
Custom(String), | ||
/// Implement `From<pyo3::PyErr>`. |
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.
Are the Rustdoc comments accurate? Starting with "Implement" I mean.
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.
Regarding thiserror
, why are we not using it in other places? @rcoh
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.
Fixed Rustdoc comments.
...oftware/amazon/smithy/rust/codegen/server/smithy/customizations/AdditionalErrorsDecorator.kt
Show resolved
Hide resolved
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
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.
LGTM!
A new generated diff is ready to view.
A new doc preview is ready to view. |
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.