-
Notifications
You must be signed in to change notification settings - Fork 196
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
RFC: Client Crate Organization #1936
Conversation
This is very much a work in progress. I put some ideas down on paper, but I'm not entirely happy with it and would love suggestions. There is a TODO in the errors section that especially needs attention. I'm wondering if, instead of what the RFC is currently proposing, we should put all code generated types inside of |
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.
overall I really like this! I think we should do some more line-by-line nitpicking at a later date
``` | ||
|
||
The `AppName` is infrequently set, and should be moved into `crate::config`. | ||
The `Error` and `ErrorExt` types should be moved into `crate::error` since |
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.
totally agree about ErrorExt
. Depending on where you land on errors, I think keeping Error
in the root so you can do without any duplicate error::Error
is probably worthwhile.
The other stuff in the root like Endpoint
should be moved. PKG_VERSION
could be moved into a meta
module. It's debatable if Region
needs to be re-exported at all now that we have aws-config.
async fn do_a_thing() -> Result<String, aws_sdk_s3::Error> -> { ... }
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.
Your reason for keeping Error
in the top-level makes sense to me.
I think Region
should still be re-exported since it's referenced by Config
and the config's Builder
. It can be moved into crate::config
though.
Empty Modules | ||
------------- | ||
|
||
The `lens` and `http_body_checksum` modules have nothing inside them, |
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 lens isn't private that's a bug 🤭
A new generated diff is ready to view.
A new doc preview is ready to view. |
The changes being proposed in this RFC will also impact server crate organization unless we do additional work to keep the server the same as it is now. Adding the |
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. |
The `AppName` is infrequently set, and should be moved into `crate::config`. The `aws-config` crate | ||
drastically reduces the usefulness of `Credentials`, `Endpoint`, and `Region`, so these can move | ||
into `crate::config`. `ErrorExt` should be moved into `crate::error`, but `Error` should stay in |
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.
The `AppName` is infrequently set, and should be moved into `crate::config`. The `aws-config` crate | |
drastically reduces the usefulness of `Credentials`, `Endpoint`, and `Region`, so these can move | |
into `crate::config`. `ErrorExt` should be moved into `crate::error`, but `Error` should stay in | |
The `AppName` is infrequently set, and should be moved into `crate::config`. We currently encourage users to import `Credentials`, `Endpoint`, and `Region` from the `aws-config` crate, so service crate reexports of these types can move into `crate::config`. `ErrorExt` should be moved into `crate::error`, but `Error` should stay in |
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.
Looking at this RFC, it's making me wonder again whether we should not also be feature-gating each operation in order to shorten compile times. We could generate the feature flags for all ops and then include them all by default. Users wanting short compile times would disable default features and include only what they use.
Decided to move the "combine |
The RFC has been updated to organize code generated types by operation. |
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. |
3748002
to
df19d7a
Compare
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
This RFC proposes some organizational changes to the generated SDK and client crates to make navigating the documentation easier while also facilitating organized re-export of runtime crate types.
Rendered
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.