tl;dr below
I love the addition of ServerError and its descendants in #350, and the further refinement in #359. (In fact I'd like to take it further and wrap Faraday::ClientError in a new class as well). However it seems wrong to me that ServerError descends from InternalError and in turn from SlackError.
Prior to #350, SlackError and its many descendants represented a specific type of error: the Web API has responded with a 200, but the body has "ok":false and "error":"some_error_code". In other words, the API has received the request, processed it, and for some reason the API cannot comply. #312 (which I authored) added the specific descendant classes for all known error codes, but still SlackError only represented 200 with "ok":false.
All of these new error classes represent wholly different situations, in some cases a 500, or in other cases a 200 with a non-JSON body (representing something like an outage). From a classification standpoint it would make more sense to give these new errors a dedicated parent class and leave SlackError to represent just the "ok":false responses.
There's also a practical consideration, which is that it appears the 3 methods defined on SlackError will raise if accessed on these new error classes, since response.body doesn't conform. (I could be wrong about this, I haven't actually tried it.) Those methods are specific to the body from a response that has "ok":false.
begin tl;dr
My suggestion is to have Slack::Web::Api::Errors::ServerError descend directly from ::Faraday::Error
(just like SlackError does). This would be a breaking change and require the appropriate version bump.
end tl;dr
Here's another advantage to this change: I saw @dblock's comment on #350 about the redundancy of passing parsing_error into new, which is necessary for the initializer on SlackError. With this new approach ServerError (and perhaps the descendants) could define its own initializer that doesn't require this.
One final note: this pattern is already established by TooManyRequestsError which descends directly from ::Faraday::Error rather than SlackError. Like the situations covered by ServerError it is not a 200 response that has "ok":false, which I assume is why it was made to not extend SlackError.
Tagging also @ojab and @agrobbin as the authors of #350 and #359, respectively.
tl;dr below
I love the addition of
ServerErrorand its descendants in #350, and the further refinement in #359. (In fact I'd like to take it further and wrapFaraday::ClientErrorin a new class as well). However it seems wrong to me thatServerErrordescends fromInternalErrorand in turn fromSlackError.Prior to #350,
SlackErrorand its many descendants represented a specific type of error: the Web API has responded with a 200, but the body has"ok":falseand"error":"some_error_code". In other words, the API has received the request, processed it, and for some reason the API cannot comply. #312 (which I authored) added the specific descendant classes for all knownerrorcodes, but stillSlackErroronly represented 200 with"ok":false.All of these new error classes represent wholly different situations, in some cases a 500, or in other cases a 200 with a non-JSON body (representing something like an outage). From a classification standpoint it would make more sense to give these new errors a dedicated parent class and leave
SlackErrorto represent just the"ok":falseresponses.There's also a practical consideration, which is that it appears the 3 methods defined on
SlackErrorwill raise if accessed on these new error classes, sinceresponse.bodydoesn't conform. (I could be wrong about this, I haven't actually tried it.) Those methods are specific to the body from a response that has"ok":false.begin tl;dr
My suggestion is to have
Slack::Web::Api::Errors::ServerErrordescend directly from::Faraday::Error(just like
SlackErrordoes). This would be a breaking change and require the appropriate version bump.end tl;dr
Here's another advantage to this change: I saw @dblock's comment on #350 about the redundancy of passing
parsing_errorintonew, which is necessary for the initializer onSlackError. With this new approachServerError(and perhaps the descendants) could define its own initializer that doesn't require this.One final note: this pattern is already established by
TooManyRequestsErrorwhich descends directly from::Faraday::Errorrather thanSlackError. Like the situations covered byServerErrorit is not a 200 response that has"ok":false, which I assume is why it was made to not extendSlackError.Tagging also @ojab and @agrobbin as the authors of #350 and #359, respectively.