-
-
Notifications
You must be signed in to change notification settings - Fork 51
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
Merge request and response types #569
Conversation
Codecov Report
@@ Coverage Diff @@
## 7.x.x #569 +/- ##
==========================================
- Coverage 78.87% 78.86% -0.02%
==========================================
Files 71 73 +2
Lines 6292 6467 +175
==========================================
+ Hits 4963 5100 +137
- Misses 1329 1367 +38
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
No need to include in this PR
ccaf8e8
to
80cd46b
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.
LGTM - couple of nits
@@ -576,52 +576,6 @@ class AWSClientTests: XCTestCase { | |||
} | |||
} | |||
|
|||
/// verify we are not calling the Retry handler when streaming a request | |||
func testDontRetryStreamingRequests() async { |
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'm assuming we don't need this test anymore? I can't see it transferred anywhere
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.
Yes we don't need it anymore. Previously I couldn't run retry on streamed responses because they called a callback for each data chunk. If you retried the request you could be sending the same data twice. Now we return the header and an async sequence with the body data and this is no longer a concern.
You can see in AWSClient.swift line 451 the related code has been deleted
* reorder execute * process response in execute * Collapse AWSHTTPResponse and AWSResponse into one * Collpase AWSRequest and AWSHTTPRequest into one * Add HTTP to request and response type names * Remove new middleware until we need it No need to include in this PR * Remove re-org to make PR review easier * Changes from PR comments * Another fix
* reorder execute * process response in execute * Collapse AWSHTTPResponse and AWSResponse into one * Collpase AWSRequest and AWSHTTPRequest into one * Add HTTP to request and response type names * Remove new middleware until we need it No need to include in this PR * Remove re-org to make PR review easier * Changes from PR comments * Another fix
* reorder execute * process response in execute * Collapse AWSHTTPResponse and AWSResponse into one * Collpase AWSRequest and AWSHTTPRequest into one * Add HTTP to request and response type names * Remove new middleware until we need it No need to include in this PR * Remove re-org to make PR review easier * Changes from PR comments * Another fix
* reorder execute * process response in execute * Collapse AWSHTTPResponse and AWSResponse into one * Collpase AWSRequest and AWSHTTPRequest into one * Add HTTP to request and response type names * Remove new middleware until we need it No need to include in this PR * Remove re-org to make PR review easier * Changes from PR comments * Another fix
AWSRequest and AWSHTTPRequest have been merged to AWSHTTPRequest
AWSResponse and AWSHTTPResponse have been merged to AWSHTTPResponse
As request doesn't include some information it had before which was used in middleware this has been moved to the middleware context.