Introduce a Standard enum for connections
#237
Conversation
| /// When testing code that uses the SDK, the variant enables using a `TestConnection` object | ||
| /// in place of a real hyper HTTP client | ||
| // Note: this variant may be removed in favor of having `TestConnection` be used via Dyn<Box<...>> | ||
| Test(TestConnection<hyper::Body>), |
There was a problem hiding this comment.
I would start w/o this and added it later if need be.
| /// 3. Any implementation of the `HttpService` trait | ||
| /// | ||
| /// This is designed to be used with [`aws_hyper::Client`](crate::Client) as a connector. | ||
| pub enum Standard { |
There was a problem hiding this comment.
I would avoid the enumeration in the public API. Keep variants private. I don't think there is a reason to list out the variants?
There was a problem hiding this comment.
For example, you could make pub struct Standard(Strategy) and have a private enum Strategy { .. }.
| #[derive(Clone)] | ||
| pub struct Standard(Connector); |
There was a problem hiding this comment.
what about StandardConnection or something like that? the name Standard is pretty vague when taken out of the module context (re-exports and the like).
There was a problem hiding this comment.
generally agree, but the other side of the house requested that I follow https://doc.rust-lang.org/1.0.0/style/style/naming/README.html#avoid-redundant-prefixes-[rfc-356] 🤷🏻
I guess we should pick and stick to a convention, but I don't have strong feelings.
There was a problem hiding this comment.
Maybe we could rename the module as connection? or connector?
There was a problem hiding this comment.
ehh, i think conn is fine. i wasn't aware of that style guide suggestion
| /// Generally, `https()` should be used instead. This constructor is intended to support | ||
| /// using things like [`TestConnection`](crate::test_connection::TestConnection) or alternative | ||
| /// http implementations. | ||
| pub fn new(connector: Box<dyn HttpService>) -> Self { |
There was a problem hiding this comment.
if this is generally only used for tests, can we make it pub(crate) ?
There was a problem hiding this comment.
it's used for external users to test their usages of the connection (eg. for integration tests I would swap in a TestConnection/RecordingConnection.
| impl Clone for Connector { | ||
| fn clone(&self) -> Self { | ||
| match self { | ||
| Connector::Https(client) => Connector::Https(client.clone()), | ||
| Connector::Dyn(box_conn) => Connector::Dyn(box_conn.clone()), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
i assume you implemented this because it couldn't be derived? i thought it would be able to...
There was a problem hiding this comment.
oh good catch. I think this was from before I added impl Clone for Box<dyn HttpService>. Removed.
Issue #, if available: #209
Description of changes:
aws_hyper::Clientis generic inC, the underlying HTTP connection. This can make it difficult to use in real-world apps because you would need a fairly complex constraint in trait bounds to accessclient.call. If you wanted to eg. put a client in a struct your practical option was to leave the C param generic. But then, if you actually wanted to use it, you needed this 6 line bound on the inner Http Service so that call would actually be in scopeTo address this shortcoming, this diff introduces a
Standardconnection (perhaps a better name could be found) that has 2 variants:Https. What you probably want most of the time.Dyn: An escape hatch for any implementation of theHttpServicetrait.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.