Revisit HttpRequestException.HttpRequestError type

[stephentoub]

We just merged a new API adding an HttpRequestError property to HttpRequestException.

public HttpRequestException(string? message, Exception? inner = null, HttpStatusCode? statusCode = null, HttpRequestError? httpRequestError = null);
public HttpRequestError? HttpRequestError { get; }

There is controversy over HttpRequestError, in particular that it's a Nullable<HttpRequestError> when HttpRequestError.Unknown also exists. There are then two different ways to communicate that a meaningful HttpRequestError value could not be provided.

I propose the above be changed to instead be:

public HttpRequestException(HttpRequestError httpRequestError, string? message = null, Exception? inner = null, HttpStatusCode? statusCode = null);
public HttpRequestError HttpRequestError { get; }

• HttpRequestError parameter to the ctor is moved to be first
• message is given a nullable default value
• HttpRequestError property is made non-nullable

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

We just merged a new API adding an HttpRequestError property to HttpRequestException.

        public HttpRequestException(string? message, Exception? inner = null, HttpStatusCode? statusCode = null, HttpRequestError? httpRequestError = null);
        public HttpRequestError? HttpRequestError { get; }

There is controversy over HttpRequestError, in particular that it's a Nullable<HttpRequestError> when HttpRequestError.Unknown also exists. There are then two different ways to communicate that a meaningful HttpRequestError value could not be provided.

I propose the above be changed to instead be:

        public HttpRequestException(HttpRequestError httpRequestError, string? message, Exception? inner = null, HttpStatusCode? statusCode = null);
        public HttpRequestError HttpRequestError { get; }

Author:	stephentoub
Assignees:	antonfirsov
Labels:	area-System.Net.Http, blocking, api-ready-for-review
Milestone:	8.0.0

[terrajobst]

This makes sense to me. Let's talk about it later but this seems like an easy change.

[terrajobst]

Video

• Makes sense as proposed

 namespace System.Net.Http;
 
 public class HttpRequestException : Exception
 {
     // Previously approved
-    public HttpRequestException(string? message, Exception? inner = null, HttpStatusCode? statusCode = null, HttpRequestError? httpRequestError = null);
-    public HttpRequestError? HttpRequestError { get; }

     // Newly approved
+    public HttpRequestException(HttpRequestError httpRequestError, string? message = null, Exception? inner = null, HttpStatusCode? statusCode = null);
+    public HttpRequestError HttpRequestError { get; }
 }