Strumenta / SmartReader

SmartReader is a library to extract the main content of a web page, based on a port of the Readability library by Mozilla
https://smartreader.inre.me
Apache License 2.0
158 stars 36 forks source link

Raise a clear exception on non-successful http status code and document it #48

Closed ivanicin closed 2 years ago

ivanicin commented 2 years ago

I haven't gone into your code, but I assume that you raise exception when HttpResponseMessage.IsSuccessStatusCode is false.

That is reasonable but this is something that is usually followed by some sort of alert to end-user in the app. However professional grade app scannot display your exception message to the end user. In this use case the package is not there to make a user-friendly message (the one you provide is fine), but rather to provide the app with enough elements to create this alert properly. So you should:

gabriele-tomassetti commented 2 years ago

Currently the library raises this exception when the it fails to fetch the content.

if (!response.IsSuccessStatusCode)
{
     throw new HttpRequestException($"Cannot GET resource {resource}. StatusCode: {response.StatusCode}");
}

So, we do not create a custom exception, we use the standard HttpRequestException with a custom message from which you can easily get the original status code.

We are not actually documenting that we could throw an exception, which is certainly bad, you are right about that. However, I do not think that we should directly create a custom exception for our library. That is because this is related to the HTTP request itself rather than anything we do. The people that are handling this case are probably handling all HTTP exceptions in the same way, they are not handling HTTP exceptions from SmartReader in a different way than HTTP exceptions from other sources.

Ideally, we should change the exception we raise to this one)), which includes a StatusCode property, but the issue is that this one is not part of the .NET Standard 2.

ivanicin commented 2 years ago

In this case too I guess it is also a lack of documentation then.