Skip to content
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

Expose InnerResponse, Http properties #1291

Merged
merged 1 commit into from
Jan 7, 2020

Conversation

piaste
Copy link
Contributor

@piaste piaste commented Oct 22, 2019

The current FSharp.Data.WebResponse wrapper does not easily allow the user to access the original response.

In particular, the WebResponse will normally (always?) be a HttpWebResponse, which exposes critical properties like StatusCode, which are currently buried pretty deep under type tests (and I might not have found them at all without debugging and looking at the source):

function 
| Error (e : Net.WebException) ->
    match e.InnerException with
    | :? Net.WebException as webExn ->
       match webExn.Response with
       | :? Net.HttpWebResponse r when r.StatusCode = Net.HttpStatusCode.Conflict -> 
         
            Log.Informationf("`Resource %s already exists, no operation needed", resourceName)
            Ok ()

This PR adds to FSharp.Data.WebResponse:

  • a set of properties that wrap those in HttpWebResponse, returning None if the wrapped response isn't a HttpWebResponse

  • an InnerResponse property that exposes the original WebResponse, for anything else a user might need

The current `FSharp.Data.WebResponse` wrapper does not allow the user to access the original response.

In particular, the `WebResponse` will normally (always?) be a `HttpWebResponse`, which exposes critical properties like `StatusCode`, which are currently inaccessible save through parsing the added line in `Message`.

This PR adds:

 - a set of properties that wrap those in [HttpWebResponse](https://docs.microsoft.com/it-it/dotnet/api/system.net.httpwebresponse?view=netframework-4.8), returning `None` if the wrapped response isn't a `HttpWebResponse` 

 - an `InnerResponse` property that exposes the original `WebResponse`, for anything a user might need.
Copy link
Contributor

@dsyme dsyme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Request adding comments. Adding a test wouldn't go astray either....

@@ -1100,6 +1105,18 @@ module private HttpHelpers =
override x.GetResponseStream () = responseStream :> Stream
member x.ResetResponseStream () = responseStream.Position <- 0L

member x.CharacterSet = httpProperty (fun r -> r.CharacterSet)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you put a /// comment on each of these? I know these are missing on the rest but we may as well not make the problem worse :)

@dsyme dsyme merged commit 15e010c into fsprojects:master Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants