bugfix: Wraps exceptional message edits in ex-info #99
Conversation
JohnnyJayJay
left a comment
There was a problem hiding this comment.
Thank you for your PR. In a comment, I have detailed what I think should be adjusted so that this bug/inconsistency is fixed entirely.
src/discljord/messaging/impl.clj
Outdated
| ~(if delete? `{} `{:body (json/write-str ~opts)}) | ||
| ~(if delete? `(= ~status 204) `(json-body ~body))))) | ||
| ~(if delete? `(= ~status 204) `(cond->> (json-body ~body) | ||
| (not= 2 (quot ~status 100)) (ex-info (str "Attempted to " ~name " with invalid parameters"))))))) |
There was a problem hiding this comment.
This condition ((not= 2 (quot ~status 100))) should really be checked in defdispatch so that it applies to (almost) all dispatch definitions, and not just message updates. The message also should rather be something like
(str "Received an error response from " name " (status " status ")") - there are more reasons for failure than just invalid parameters and the status should be mentioned.
This change requires a bit more effort because:
- existing
ex-infowrappings like inbulk-overwrite-global-application-commands!need to be removed - not every dispatch implementation is written using
defdispatch, although I think the only separate one to look out for is thesend-message!private function.
If you don't have time to make this change, just say the word and I'll make it myself. But as it stands, this only fixes a part of the inconsistency, ideally all of it should be fixed.
There was a problem hiding this comment.
Makes sense. Just pushed, I'm giving it a test now.
IGJoshua
left a comment
There was a problem hiding this comment.
Sorry to bounce you back and forth on this @kiranshila, wait to update this PR till we get this decided.
@JohnnyJayJay we should consider leaving the call to ex-info as specific to each usage of defendpoint. Many of the endpoints have the value determined as (= status 204) or similar, and those don't have a value to put into an ex-info, which can only take maps, and for these endpoints a non-200 response isn't an error as much as just a negative response.
I think that the original PR updating def-message-dispatch fixes most of the inconsistency, and we should look for specific endpoints that are inconsistent and list them as the ones that need to be fixed, rather than moving it into defendpoint.
Fixes #97