ServerError does not descend from InternalError by jmanian · Pull Request #380 · slack-ruby/slack-ruby-client

jmanian · 2021-06-19T01:01:41Z

Resolves #373.

Notes:

The change in class hierarchy is a breaking change for anyone who might be rescuing SlackError and expecting it to rescue ServerError.
I took the suggestion from Handle server errors such as timouts & non-json responses #350 about syncing the message into the class definition rather than passing it in. This is a breaking change for anyone instantiating these errors in their code.
I tweaked TooManyRequestsError to follow the same pattern for defining its message. Not a breaking change, just implementation.

Questions:

Is it OK for these 5 error classes to share a file now that they're no longer empty definitions?
I didn't do it here, but I'm tempted to change these 3 error messages to something more human readable rather than an underscore code. I assume the underscore messages were inspired by the error messages that Slack returns, but since these are not error messages from Slack they don't need to conform, and in fact it could be misleading if they look like they might be messages from Slack. For example UnavailableError could be "The server responded with #{status}". Thoughts on this? It could be considered a breaking change.
I didn't do it yet, but I'm tempted to follow the same message pattern for all the descendants of SlackError defined in errors.rb, so that instead of Slack::Web::Api::Errors::AccountInactive.new('account_inactive', response) it's simply Slack::Web::Api::Errors::AccountInactive.new(response). This would be a breaking change for anyone instantiating these error classes.

To be done later, if this approach is good:

Add to changelog
Add an entry to upgrading.md
Move the version to 0.18.0 since this is a breaking change

dblock

This looks good. Care to cleanup my minor nits and I'll merge? Thank you.

dblock · 2021-06-21T14:02:52Z

+### 0.18.0 (Next)

 * Your contribution here.
+* [#380](https://github.com/slack-ruby/slack-ruby-client/pull/380): Updates to server error classes and hierarchy (see [UPGRADING](UPGRADING.md) for more) - [@jmanian](https://github.com/jmanian).


No need to mention UPGRADING here, or our CHANGELOG will be littered with them.

dblock · 2021-06-21T14:03:26Z

 ##### Other Errors

-In case of Slack temporarily unavailability a `Slack::Web::Api::Errors::ServerError` (`Slack::Web::Api::Errors::SlackError` subclass) subclasses will be raised and original `Faraday::Error` will be accesible via `exception.cause`.
+In case of Slack temporarily unavailability a subclass of `Slack::Web::Api::Errors::ServerError` will be raised and original `Faraday::Error` will be accesible via `exception.cause`. (This is no longer a subclass of `Slack::Web::Api::Errors::SlackError`, see [UPGRADING](UPGRADING.md).)


Let's write "Since version X.Y.Z this class is no longer ... and remove see UPGRADING.

dblock · 2021-06-21T14:03:54Z

+
+As of 0.18.0 `Slack::Web::Api::Errors::ServerError` and its subclasses (introduced in 0.16.0) no longer extend `Slack::Web::Api::Errors::InternalError` or its parent `Slack::Web::Api::Errors::SlackError`. If you are rescuing `SlackError` or `InternalError` with the intention of including `ServerError` and its subclasses you should adjust your code to explicitly rescue `Slack::Web::Api::Errors::ServerError`.
+
+Additionally the `initialize` method for `ParsingError`, `TimeoutError`, and `UnavailableError` have changed from `new(message, response)` to `new(response)`. The `message` is now built into the definition of these classes. If you are instantiating or raising these errors in your code (perhaps in tests) you will need to update your code.


Maybe add some before/after?

You mean example code before and after? I can do that

jmanian · 2021-06-21T15:15:31Z

@dblock Where's Travis?

dblock · 2021-06-21T15:51:27Z

@dblock Where's Travis?

Not sure. Did they turn off travis-ci.org finally? Want to move us to GitHub actions?

dblock · 2021-06-21T15:54:46Z

@jmanian I enabled travis-ci.com on the org, maybe next push will "just work"? Let's upgrade badges...

jmanian · 2021-06-22T21:29:51Z

@dblock Still no Travis. I originally opened this PR as a draft, maybe that confused it? I authed into Travis to see if I could do anything but the repo doesn't appear for me there.

dblock · 2021-06-23T04:39:33Z

@jmanian ~~I am seeing https://travis-ci.com/github/slack-ruby/slack-ruby-client and it's enabled for PRs. Try making a new PR? Maybe that will kick it? I dunno. Rewrite everything in GitHub actions? :)~~

I tried to trigger a build, which failed with some unexpected error. So I clicked a bunch of buttons on travis-ci.com: added a free plan, signed up for "Beta", migrated repos to travis-ci.com. Then the trigger seems to have worked.

Try pushing again?

PS: All this seems unnecessary. Should get off Travis.

jmanian · 2021-06-23T13:55:42Z

@dblock Success!

dblock · 2021-06-23T21:47:51Z

Merged, merci.

ServerError does not descend from InternalError

8dcd153

jmanian requested a review from dblock June 19, 2021 01:01

jmanian linked an issue Jun 19, 2021 that may be closed by this pull request

Suggestion: ServerError should not descend from InternalError or SlackError #373

Closed

jmanian added 5 commits June 19, 2021 14:42

rename internal_error.rb to server_error.rb

3ff17b7

bump minor version

1f6d2a0

update docs

c57ed5a

fix tests for rate limiting

b6de06d

raise syntax for rubocop

1f8ce17

jmanian marked this pull request as ready for review June 19, 2021 19:22

dblock approved these changes Jun 21, 2021

View reviewed changes

update docs

692fb1e

empty commit (hello travis?)

f53cd9c

dblock merged commit 7d53b51 into slack-ruby:master Jun 23, 2021

jmanian deleted the server_error branch June 28, 2021 16:02

olleolleolle added a commit to olleolleolle/slack-ruby-client that referenced this pull request Nov 16, 2021

CHANGELOG: Note slack-ruby#380

8bbf75d


		As of 0.18.0 `Slack::Web::Api::Errors::ServerError` and its subclasses (introduced in 0.16.0) no longer extend `Slack::Web::Api::Errors::InternalError` or its parent `Slack::Web::Api::Errors::SlackError`. If you are rescuing `SlackError` or `InternalError` with the intention of including `ServerError` and its subclasses you should adjust your code to explicitly rescue `Slack::Web::Api::Errors::ServerError`.

		Additionally the `initialize` method for `ParsingError`, `TimeoutError`, and `UnavailableError` have changed from `new(message, response)` to `new(response)`. The `message` is now built into the definition of these classes. If you are instantiating or raising these errors in your code (perhaps in tests) you will need to update your code.

Uh oh!

Conversation

jmanian commented Jun 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dblock left a comment

Choose a reason for hiding this comment

Uh oh!

dblock Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

dblock Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

dblock Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

jmanian Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

dblock Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

jmanian commented Jun 21, 2021

Uh oh!

dblock commented Jun 21, 2021

Uh oh!

dblock commented Jun 21, 2021

Uh oh!

jmanian commented Jun 22, 2021

Uh oh!

dblock commented Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmanian commented Jun 23, 2021

Uh oh!

dblock commented Jun 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jmanian commented Jun 19, 2021 •

edited

Loading

dblock commented Jun 23, 2021 •

edited

Loading