Extract ApiException creation from SendRequestAsync method

[TobiasBreuer]

@canton7 : Before creating a PR I would like to get your opinion on that topic.
Currently the creation of the ApiException is part of the SendRequestAsync method (in case it is enabled).

I do have the requirement to adapt exactly this part as we are using our own ApiException class due to reasons.
For this scenario, I currently see two options:

1. use a try-catch block to catch the ApiException and then rethrow our own exception type (not nice!)
2. inherit from Requester, override the SendRequestAsync method
   2a. Call base.SendRequestAsync in try-catch block (basically the same as option 1)
   2b. Copy paste the existing code in that method and only replace the exception handling part (avoids the unnecessary try-catch, but also not nice, because when there are changes in the original code, we would have to check on every update of the package if we need to align the copied code.

Hence, the preferrable solution from my point of view would be to create a PR that refactors the method by exctracting lines 473 and 474:

var deserializer = new ApiExceptionContentDeserializer(this, response, requestInfo);
throw await ApiException.CreateAsync(message, response, deserializer).ConfigureAwait(false);

into a seperate virtual method e.g.:

...
if (!response.IsSuccessStatusCode && !requestInfo.AllowAnyStatusCode)
{
    throw await CreateExceptionForNonSuccessfulRequest(response, requestInfo, message).ConfigureAwait(false);
}
...

protected virtual async Task<Exception> CreateExceptionForNonSuccessfulRequest(HttpResponseMessage response, IRequestInfo requestInfo, HttpRequestMessage message)
{
    var deserializer = new ApiExceptionContentDeserializer(this, response, requestInfo);
    return await ApiException.CreateAsync(message, response, deserializer).ConfigureAwait(false);
}

In this case we could again inherit from Requester but only override this new method and keep everything else as is, avoiding again the non-required try-catch approach.
What do you think?
Thanks in advance and best regards!
Tobi

[canton7]

Sure, sounds sensible. Thanks for checking in first.

I'm generally trying to move away from asking people to subclass Requester these days, as subclassing is quite a brittle form of customisation and it means it's very hard to make changes to Requester without breaking people. However for a single person doing a niche thing I think it's fine, as long as you're OK with potentially having to deal with breaks (i.e. changes in method signature etc, rather than having to take an entirely different approach) in the future.

Why not extract the whole of:

            if (!response.IsSuccessStatusCode && !requestInfo.AllowAnyStatusCode)
            {
                var deserializer = new ApiExceptionContentDeserializer(this, response, requestInfo);
                throw await ApiException.CreateAsync(message, response, deserializer).ConfigureAwait(false);
            }

Then the method can become "Figure out whether this response is good, and throw if it isn't", which seems like a more sensible thing to try and extract.

[TobiasBreuer]

Thanks for your feedback!
I do agree that subclassing is not the best approach, but currently seems like the only way to go (without refactoring Requester entirely and injecting the "adaptable" parts from outside (e.g. in the case given, it would sure be nicer if Requester would make use of an IResponseHandler which we could provide our own implementation of and inject it into a DI container or into the RequesterFactory) but this is like a major refactoring within the library...
For the time being, I would take the approach discussed here and create a PR for it next week...
Best regards!