Exception refactoring - Githubissues

nozzlegear / ShopifySharp

ShopifySharp is a .NET library that helps developers easily authenticate with and manage Shopify stores.

https://nozzlegear.com/shopify-development-handbook

MIT License

742 stars 308 forks source link

Exception refactoring #996

Closed nozzlegear closed 7 months ago

nozzlegear commented 7 months ago

This pull request does a light bit of refactoring on ShopifySharp's exceptions:

Introduces a new exception type named ShopifyHttpException, which inherits from the base ShopifyException. This new exception should be thrown whenever an http request fails or returns an error.
Made the ShopifyRateLimitException inherit from the ShopifyHttpException.
Deprecates most HTTP properties on the base ShopifyException:
- HttpStatusCode
- RequestId
- Errors
- RawBody
Removes the deprecated ShopifyException.HttpResponse property.
Refactors the base ShopifyService, GraphService and PartnerService classes to throw a ShopifyHttpException where applicable.
Removes a custom error message that was injected into the ShopifyException.Errors string list in some cases. This error message was an exact duplicate of the exception's message.

clement911 commented 7 months ago

This will be an issue for us as we use the properties that you have deprecated. Specifically HttpStatusCode and RequestId and RawBody

nozzlegear commented 7 months ago

@clement911 Understood, hopefully this won't be the case. My intent wasn't to get rid of those properties entirely! They're still available on the new ShopifyHttpException class where I plan to keep them. Anywhere you use the ShopifyException class with the three deprecated properties, you should be able to switch it to ShopifyHttpException and the deprecation warnings will disappear.

My intent is that the base ShopifyException is thrown for things like configuration errors; ShopifyHttpException is thrown for errors that occur with http requests and error responses from Shopify; and ShopifyRateLimitException is thrown when hitting the rate limit.

Does that make sense?

clement911 commented 7 months ago

Oh I see, that should be fine then.

clement911 commented 7 months ago

Although I see that ShopifyHttpException is under the ShopifySharp.Infrastructure. Personally I'd prefer to have a single ShopifySharp namespace.

nozzlegear commented 7 months ago

Oh woops, I don't think I meant to put it there. Might have been Rider sneaking that in. I'll change it!