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

Update error message for GeneralHttpException to include the status code #530

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

revam
Copy link
Contributor

@revam revam commented Jan 20, 2025

Updated the error message for the GeneralHttpException to include the status code, since I just observed a wild GHE thrown on a system outside my control and couldn't for the love of all computers find out which status code it had, even though I've tried to reproduce it and have since added GHE as an expected exception to handle in the retry policy in the app. Just wish I had the status code so I knew why it threw on that one system outside my control, so here I am, adding it for any future devs in my previous position. Feel free to suggest another format if my suggested one is not accepted. Or just reject it outright, but if you do reject it, do please explain why it's rejected.

… code.

Updated the error message for the `GeneralHttpException` to include the status code, since I just observed a wild GHE thrown on a system outside my control and couldn't for the love of all computers find out which status code it had, even though I've tried to reproduce it and have since added GHE as an expected exception to handle in the retry policy in the app. Just wish I had the status code so I knew _why_ it threw on that one system outside my control, so here I am, adding it for any future devs in my previous position. Feel free to suggest another format if my suggested one is not accepted. Or just reject it outright, but if you do reject it, do please explain _why_ it's rejected.
@@ -7,7 +7,7 @@ public class GeneralHttpException : TMDbException
public HttpStatusCode HttpStatusCode { get; }

public GeneralHttpException(HttpStatusCode httpStatusCode)
: base("TMDb returned an unexpected HTTP error")
: base($"TMDb returned an unexpected HTTP error: {(int)httpStatusCode}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need the explicit conversion to int?

Copy link
Contributor Author

@revam revam Jan 20, 2025

Choose a reason for hiding this comment

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

I did that to be on the safe side in case a status code returned from upstream is not part of the HttpStatusCode enum. If we don't cast to an int (or any other numerical type for that matter), then it will try to convert the value to the value's name when it stringifies it, but I don't and didn't remember how the runtime handles out of bounds values when stringifying, so for consistency's sake I would rather have it always print out the status code in it's numerical form. If you feel it's too much then we can drop the type cast.

An example of a status code not part of the enum (within is codebase even) is 429 (too many requests).

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.

3 participants