-
Notifications
You must be signed in to change notification settings - Fork 11
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
Return the HTTP response status and headers #108
base: 3.0.0
Are you sure you want to change the base?
Conversation
Callers should have access to the response headers - specifically the x-shipengine-request-id.
ShipEngineSDK/Api/AccountApi.cs
Outdated
var (data, response) = await GetHttpResponse<GetAccountSettingsImagesResponseBody>(HttpMethods.Post, requestOptions.FullPath(), requestOptions.Data, methodClient, _config, cancellationToken); | ||
var headers = response.Headers.ToDictionary(h => h.Key, h => h.Value.FirstOrDefault(), | ||
StringComparer.InvariantCultureIgnoreCase); | ||
return new ShipEngineResponse<GetAccountSettingsImagesResponseBody>(data, response.StatusCode, headers); |
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.
Am I reading this wrong or does this make the data inaccessible? From what I saw, the ShipEngineResponse
holds the data in a private field.
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.
Oh, you're right. That field should be public. This was late-night sketching, I haven't run any of it yet - just stopped when I got it to compile.
ShipEngineSDK/Api/AccountApi.cs
Outdated
var (data, response) = await GetHttpResponse<GetAccountSettingsImagesResponseBody>(HttpMethods.Post, requestOptions.FullPath(), requestOptions.Data, methodClient, _config, cancellationToken); | ||
var headers = response.Headers.ToDictionary(h => h.Key, h => h.Value.FirstOrDefault(), | ||
StringComparer.InvariantCultureIgnoreCase); | ||
return new ShipEngineResponse<GetAccountSettingsImagesResponseBody>(data, response.StatusCode, headers); |
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.
Would all this be worth doing inside the client? I know this is autogenerated code so the repetition isn't a big deal, but it seems like core functionality that all consumers do.
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, it would be better done in the client. Now that I am using a new method GetHttpResponse
anyway, that should be possible. (originally tried to do it using the existing SendHttp...)
ShipEngineSDK/ShipEngineResponse.cs
Outdated
@@ -0,0 +1,18 @@ | |||
namespace ShipEngineSDK; |
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.
Just a formatting beef, but I think most of our code puts the namespace right above the class declaration and after the using
statements (even though I know some tools don't like doing this)
Callers should have access to the response headers - specifically the x-shipengine-request-id.
The significant changes are in
ShipEngineClient
,ShipEngineResponse
andapi.mustache
. The rest are the result of regeneration.