redis-rb / redis-client

Simple low level client for Redis 6+
MIT License
125 stars 60 forks source link

Should RedisClient::TimeoutError descend from Timeout::Error? #19

Closed mperham closed 2 years ago

mperham commented 2 years ago

Seems weird to raise network timeout errors which are not catchable by a standard rescue Timeout::Error. Does it make sense to extend Ruby's timeout error to gain compatibility? Or maybe just raise Timeout::Error directly? Is there a benefit to defining your own class vs reusing stdlib errors?

casperisfine commented 2 years ago

Is there a benefit to defining your own class vs reusing stdlib errors?

Yes, the benefit is to have one base error, so that if you wish to handle any possible redis error rescue RedisClient::Error is all you need. In my mind there's nothing worse than APIs that leaks underlying exceptions (e.g. net/http leaks a stupid amount of errors).

It can also be done using modules, but I'd rather avoid it.

Note that redis does the same.

I'm curious what advantage you find to re-using (or subclassing) stdlib errors.

mperham commented 2 years ago

I'm thinking of generic network error handling, e.g. rescue Timeout::Error, Errno::ETIMEOUT. I don't have a real-world usecase here but I feel a bit of redundancy in the code. I'm not sure about the right thing to do but I'm also ok with you closing this if you don't feel the same.