Should this class be sealed? Or is there a reason to derive from it?

Agree, no reason to derive from it

It looks like this property is never set in the error handling code below. Does this just return the response.StatusCode?

Was originally adding the individual properties before I saw the example of Windows PowerShell just having a Response property. Didn't remove them. Will remove.

Same for this property. Is this _response.Headers?

Will remove.

I believe we normally use automatic implemented getter/setter property syntax:

public HttpResponseMessage Response { get; set; }

Will fix

You could use this syntax here for creating the HttpResponseException:

var httpEx = new HttpResponseException(message)
{
    Response = response,
    Status = ...
    Headers = ...
};

Or you could make the constructor take all property values and make the properties read only.

Will change constructor

Should we test for Exception.Headers and Exception.Status too?

Those are getting removed

We should make the setter private { get; private set; }. Otherwise LGTM.

I agree. WebException.Response only has a getter.

Makes sense

I suggest we make HttpResponseException derive from HttpRequestException, so that { Invoke-WebRequest <url> } catch [HttpRequestException] { } will continue to work after this change.

Agree

The message for ErrorDetails should be the content of the response (after using regex replace to remove tags). This was what #3089 tried to fix, and the corresponding code in full powershell is here

Will make the change

pls add a test with a response content

will do

Exceptions may be thrown out in StreamHelper.GetResponseStream(response) or reader.ReadToEnd(). We should put the using block in a try/catch-all block, just like what we did in full ps.

ok

Convert.ToInt32(response.StatusCode).ToString()

A minor comment: maybe just (int)response.StatusCode as in HttpResponseMessage.cs?
If we want to avoid boxing the int argument, then it should be ((int)response.StatusCode).ToString(), but maybe it's an overkill 😄

I'm ok with just the cast

er.ErrorDetails should be set only if detailMsg is not an empty string..

ok

Minor comment: this might fail if we are going to run this test on a localized machine.

Dongbo Wang (@daxian-dbw) Is a match good enough for the part returned from the server?

changed to Match

Yes, I think so, $result.Error.Exception.Message | Should BeLike * 418 (I'm a teapot) should be good, since they are returned from the Http service, not affected by PS localization.

Same minor comment here.

changed to Match

I suggest moving the class into a separate file

I did a search of the existing source and it appears that in some cases Exceptions are declared in a separate file (like SessionStateExceptions.cs), but in many other cases, the Exception is declared where it's used (like parserutils.cs).

I suggest using try-catch-finally to avoid nesting

ok

Max Maximov (@2xmax) do you mean like this?

StreamReader reader = null;
try
{
  reader = new StreamReader(StreamHelper.GetResponseStream(response))
  ....
}
catch { }
finally { if (reader != null) { reader.Dispose(); } }

I'm afraid you need to put new StreamReader(StreamHelper.GetResponseStream(response)); in the try block. Maybe like this:

string detailMsg = "";
StreamReader reader = null;
try
{
  reader = new StreamReader(StreamHelper.GetResponseStream(response))
  detailMsg = System.Text.RegularExpressions.Regex.Replace(reader.ReadToEnd(), "<[^>]*>","");
}
catch { }
finally { if (reader != null) { reader.Dispose(); } }

fixed

And this file got committed by accident 😄

fixed