Handle server errors such as timouts & non-json responses

[ojab]

My stab @ #306
It's really a draft, but I want some feedback since I'm not really used to slack-ruby-client, so making it non-draft.

[ojab]

We can't add this stuff into on_complete like it's done in #307 because on_complete is not called in that cases, we just raising from inner middleware.

[ojab]

I think that specs from here should be moved to Slack::Web::Client and use vcr/be more integration-like than unit-like, please advise.
But right now we're not testing what really happens when server responds with 500/timeout/non-json.

[dblock]

I think TooManyRequestsError is an SlackError so both here would get rescued with just SlackError

[ojab]

> Slack::Web::Api::Errors::TooManyRequestsError <  Slack::Web::Api::Errors::SlackError
=> nil

[dblock]

Ah yes because we inherited this one from Faraday::Error to get retry_after.

[dblock]

Maybe there needs to be an intermediate Slack::Web::Api::Errors::HttpRequestError that holds an inner FaradayError exception?

[dblock]

Needs a clean green build please, README and UPGRADING updates. Otherwise LGTM.

[dblock]

I would sink parsing_error message into the error definition itself when calling super

[ojab]

Not sure what do you mean here.

[dblock]

Like this. I don't think there's some kind of convention that the first argument must be a string, but I could be wrong? This is not a must have change for me here.

raise Slack::Web::Api::Errors::ParsingError.new(env.response)

[ojab]

As a user of this gem I'm expecting first argument (i. e. message) of Slack::Web::Api::Errors::Error to be a kinda meaningful String, while the second is response, that's how it works everywhere currently. So I assume that there actually is an implicit convention.
And while it will be more appropriate to catch & process Slack::Web::Api::Errors::ParsingError separately — I assume something like

rescue Slack::Web::Api::Errors::Error => e
  raise unless e.message == 'parsing_error'

also is expected to work.

But I also don't have strong feelings about this.

[dblock]

My issue is that it's an argument to the constructor that hides the message argument. I was thinking like so:

class TimeoutError < HttpRequestError
  def initialize(response)
     super 'timeout_error'
  end
end

raise Slack::Web::Api::Errors::TimeoutError

[dblock]

See my comments, expand on UPGRADING, add a CHANGELOG entry and this is good to go. Thanks for hanging in here!

[dblock]

Like this. I don't think there's some kind of convention that the first argument must be a string, but I could be wrong? This is not a must have change for me here.

raise Slack::Web::Api::Errors::ParsingError.new(env.response)

[dblock]

Needs a CHANGELOG entry otherwise good to go with or without the message/super change.

[ojab]

CHANGELOG is added.

[dblock]

Merged