Introduce UnknownError

[twe4ked]

Hello!

Rather than raising ResponseError, which is also used as a base class for other errors, we now raise UnknownError.

This results in more specific error reporting when an unknown error occurs. For example, you can now handle unknown errors separately to ResponseError exceptions.

Compatibility:

Because UnknownError is still a subclass of ResponseError, if you are currently rescuing ResponseError or using Object#is_a?, your code will still work. If you're checking against the specific constant you will need to update your code.

Why?

If you're handling errors from the library by rescuing them like this:

begin
  WebPush.payload_send(…)
rescue WebPush::InvalidSubscription, WebPush::ExpiredSubscription
  # Handle invalid subscriptions in some way (we mark them as deleted)
rescue WebPush::ResponseError => e
  # Handle all known response errors in the same way.
  #
  # This will also catch "unknown errors" which aren't expected by the library.
rescue Errno::ECONNRESET, Net::OpenTimeout, OpenSSL::SSL::SSLError
  # Handle connection errors
end

Because the unknown error is just a WebPush::ResponseError it's more difficult to handle it differently without explicitly rescuing all the other kinds of WebPush::ResponseError earlier. With this change you can rescue WebPush::UnknownError before the generic WebPush::ResponseError and handle it separately. Unknown errors can be important to look at separately rather then rescuing them along with all the other known errors.

Cheers

[collimarco]

This is your description:

Because UnknownError is still a subclass of ResponseError

This is your code:

class UnknownError < StandardError; end

There is definitely something wrong in your pull request.

[twe4ked]

Thanks, fixed.

[collimarco]

Thanks for fixing that, however I don't understand why this would be useful... When you use this gem, you first rescue from the specific exceptions and finally you rescue from the generic ResponseError. I don't see any advantage in the proposed change. Furthermore, it can introduce some backward-compatibility issues.

[twe4ked]

Sorry about that, I've updated the description to try to explain.

Essentially I'd like to be able to handle “unknown errors” separately first and then rescue ResponseError later to handle some of the known errors in a generic way. I wasn't expecting having unknown errors come through as generic response errors, I would have assumed they'd all be expected/handled error types.

I can work around this without the change so happy for you to close if you still don't see any advantage.