Raise system error subclasses #53
Conversation
#41 added a translation of `ECONNREFUSED` and `ECONNRESET` to `Trilogy::BaseConnectionError`. We've got a few places at GitHub where we explicitly rescue these errors (or `SystemCallError`). Although it is possible to rework those rescues to check for particular error messages (like trilogy-libraries/activerecord-trilogy-adapter@03c1610), it'd be a bit easier if we could maintain compatibility. I also worry that checking for specific error messages could be a bit brittle. This commit introduces two new Trilogy errors, which inherit from the corresponding `Errno` errors. We've already done something like this for `Errno::ETIMEDOUT`. This keeps Trilogy compatible with GitHub's existing rescues, while still raising something that is a `Trilogy::Error` and a `Trilogy::ConnectionError` (although NOT a `Trilogy::BaseConnectionError` anymore). A downside to this approach is that we might end up with a lot of Trilogy-specific `Errno` subclasses. If that gets annoying perhaps we could revisit when it comes time for a major release. Another approach might be a single `Trilogy::SystemCallError` that inherits from `SystemCallError` and then keeps a reference to the underlying `Errno` error.
There was a problem hiding this comment.
Makes sense to me!
As a heads up, I was planning to submit a PR in the next week or so with proposed breaking changes to the error classes (ie. moving away entirely from the syscall errors) in anticipation of Trilogy being upstreamed. Makes sense to have these backwards compatible changes shipped ahead of that major version bump though 👍 Is your team still on board with those changes taking place?
|
Yeah, I think it's fine to make further breaking changes. If the changes here are going in the wrong direction, I can try something else. The main thing is that we'd like to rescue these separately from other Trilogy errors, so |
|
Maintaining the parallel to the way we've handled |
#41 added a translation of
ECONNREFUSEDandECONNRESETtoTrilogy::BaseConnectionError.We've got a few places at GitHub where we explicitly rescue these errors (or
SystemCallError). Although it is possible to rework those rescues to check for particular error messages (like we did intrilogy-libraries/activerecord-trilogy-adapter@03c1610), it'd be a bit easier if we could maintain compatibility. I also worry that checking for specific error messages could be a bit brittle.
This commit introduces two new Trilogy errors, which inherit from the corresponding
Errnoerrors. We've already done something like this forErrno::ETIMEDOUT.This keeps Trilogy compatible with GitHub's existing rescues, while still raising something that is a
Trilogy::Errorand aTrilogy::ConnectionError(although NOT aTrilogy::BaseConnectionErroranymore).A downside to this approach is that we might end up with a lot of Trilogy-specific
Errnosubclasses. If that gets annoying perhaps we could revisit when it comes time for a major release. Another approach might be a singleTrilogy::SystemCallErrorthat inherits fromSystemCallErrorand then keeps a reference to the underlyingErrnoerror.cc @adrianna-chang-shopify