ruby: Base error types?

[casperisfine]

Currently the gem raise many different classes of errors, from what I gathered Trilogy#query can raise:

• Errno::ETIMEDOUT on timeout
• IOError when trying to use a closed connection.
• Trilogy::Error for casting errors (and others?)

With database client it's generally very useful to have one base exception type that can be rescued, e.g. Redis::BaseConnectionError, otherwise it's too easy to forget one of the possible type of error on query.

Backward compatibility

To retain backward compatibility it would be possible to use modules, e.g.:

module Trilogy
   ConnectionErrors = Module.new

   class TimeoutError < Errno::ETIMEDOUT
     include ConnectionErrors
   end

   class ConnectionClosed < IOError
     include ConnectionErrors
   end

This would both allow to rescue all connection related errors with rescue Trilogy::ConnectionErrors while retaining compatibility with existing code using rescue Errno::ETIMEDOUT, IOError.

What do you think?

[matthewd]

Yeah, I do like inheriting from the ruby core types where applicable, so the module approach sounds good to me.

What do you think of making Trilogy::Error itself a catch-all module for everything the gem can raise?

[casperisfine]

What do you think of making Trilogy::Error itself a catch-all module for everything the gem can raise?

Would be great yes. For network clients l like to have a catch all like you suggest, then a base for everything "transcient" typically network issues, then another base for "invalid statements" type of errors.

If it's OK to change Trilogy::Error to be a module instead of a class, then I can do that.

[composerinteralia]

Looks like GitHub does have some tests in a few places that do raise Trilogy::Error.new, but we should be able to change those to raise a more specific error.

Changing it from a class to a module may have implications for the version number the next time we release.

[casperisfine]

Changing it from a class to a module may have implications for the version number the next time we release.

Yeah, an alternative is to have a Trilogy::Errors module alonside.

[matthewd]

As long as it's still rescuable, I think it's probably fine? Technically even introducing and raising a subclass is an API change, but that seems like an over-constraint, especially right now while we can be pretty confident there are few downstreams to be potentially affected.

[casperisfine]

Sounds good, I'll submit a PR tomorrow then.

[casperisfine]

What do you think of making Trilogy::Error itself a catch-all module for everything the gem can raise?

So there's a bit of a problem with that one. Many errors are directly raised as Trilogy::Error directly from mysql error codes. So we'd need to distinguish between "client errors" and "server errors".