-
Notifications
You must be signed in to change notification settings - Fork 189
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
Remove Protocol enum #1829
Remove Protocol enum #1829
Conversation
0469b3b
to
236d6ea
Compare
{ | ||
Self { | ||
handlers, | ||
into_response: PyMiddlewareException::into_response, |
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 store the into_response
here as a fn
to immediately erase the P
type. This prevents any further generics in the parent structs.
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.
Sweet!
"aws.protocols#awsjson10" => Protocol::AwsJson10, | ||
"aws.protocols#awsjson11" => Protocol::AwsJson11, | ||
_ => { | ||
return Err(PyException::new_err(format!( |
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 fallibility is gone. It's replaced by the bound PyMiddlewareException: IntoResponse<P>
on the Layer::layer
below. We can move it up to the constructor if we want the compile error more eagerly.
|
||
/// Tower [Layer] implementation of Python middleware handling. | ||
/// | ||
/// Middleware stored in the `handlers` attribute will be executed, in order, | ||
/// inside an async Tower middleware. | ||
#[derive(Debug, Clone)] | ||
pub struct PyMiddlewareLayer { | ||
pub struct PyMiddlewareLayer<P> { |
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.
TODO: Add documentation on what this P
is. What is it, where do you find it etc?
@@ -63,8 +63,6 @@ pub trait PyApp: Clone + pyo3::IntoPy<PyObject> { | |||
|
|||
fn middlewares(&mut self) -> &mut PyMiddlewares; | |||
|
|||
fn protocol(&self) -> &'static str; |
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've removed this because there's currently no method to go from struct AwsRestJson1
(etc) to "aws.protocols#restJson1"
. This can be easily added though if desired.
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.
Or we can simply write a code generated literal in the implementation.
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 protocol
method in this trait was a hack to be honest. I like what you did here.
...are/amazon/smithy/rust/codegen/server/python/smithy/generators/PythonApplicationGenerator.kt
Show resolved
Hide resolved
693c15e
to
fd100dd
Compare
A new generated diff is ready to view.
A new doc preview is ready to view. |
Fix missing import Fix missing markerStruct
12f9149
to
2c8f536
Compare
fc3b3a3
to
4f7fc07
Compare
Fix python tests Restore default Add missing codegenScope Fix missing new::<P>
4f7fc07
to
043c06f
Compare
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 gave it a quick skim and lgtm Harry!
There are still some failure in CI, but they are trivial misses. |
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. |
Motivation and Context
The
Protocol
enum defines a closed set of protocols supported by therust-runtime
crates. Its use within interfaces restricts extensibility.While this is not a solution to #1703 it does progress towards the general goal.
Description
Protocol
enum fromaws-smithy-http-server
.Protocol
acrossaws-smithy-http-server
.IntoResponse<P>
on theRuntimeError
.RuntimeError
and renameRuntimeErrorKind
toRuntimeError
.Protocol
acrossaws-smithy-http-server-python
.P
withPyMiddlewareException: IntoResponse<P>
. It is no longer fallible.PyApp::protocol
method.Notes
The most contentious changes occur within the
aws-smithy-http-server-python
changes.