Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new error classes #841

Merged
merged 2 commits into from
Jan 4, 2019
Merged

Add new error classes #841

merged 2 commits into from
Jan 4, 2019

Conversation

iMacTia
Copy link
Member

@iMacTia iMacTia commented Jan 4, 2019

Description

  • Removes sub-class constants definition from Faraday::Error. A sub-class (e.g. ClientError) was previously accessible either through the Faraday module (e.g. Faraday::ClientError) or through the Faraday::Error class (e.g. Faraday::Error::ClientError).
    The latter is no longer available and the former should be used instead, so check your rescues.
  • Introduces a new Faraday::ServerError (5xx status codes) alongside the existing Faraday::ClientError (4xx status codes).
    Please note Faraday::ClientError was previously used for both.
  • Introduces new Errors that describe the most common REST status codes:
    • Faraday::BadRequestError (400)
    • Faraday::UnauthorizedError (401)
    • Faraday::ForbiddenError (403)
    • Faraday::ProxyAuthError (407). Please note this raised a Faraday::ConnectionFailed before.
    • Faraday::UnprocessableEntityError (422)

Todos

List any remaining work that needs to be done, i.e:

  • Tests
  • Documentation
  • UPGRADING.md
  • Fix Typhoeus adapter

Additional Notes

Replaces #735

…-class (e.g. `ClientError`) was previously accessible

either through the `Faraday` module (e.g. `Faraday::ClientError`) or through the `Faraday::Error` class (e.g. `Faraday::Error::ClientError`).
The latter is no longer available and the former should be used instead, so check your `rescue`s.
* Introduces a new `Faraday::ServerError` (5xx status codes) alongside the existing `Faraday::ClientError` (4xx status codes).
Please note `Faraday::ClientError` was previously used for both.
* Introduces new Errors that describe the most common REST status codes:
  * Faraday::BadRequestError (400)
  * Faraday::UnauthorizedError (401)
  * Faraday::ForbiddenError (403)
  * Faraday::ProxyAuthError (407). Please note this raised a `Faraday::ConnectionFailed` before.
  * Faraday::UnprocessableEntityError (422)
@iMacTia iMacTia added the v1.0 label Jan 4, 2019
@iMacTia iMacTia added this to the v1.0 milestone Jan 4, 2019
@iMacTia
Copy link
Member Author

iMacTia commented Jan 4, 2019

@olleolleolle you're welcome to have a review 👍 !

@iMacTia iMacTia removed the v1.0 label Jan 4, 2019
iMacTia added a commit to iMacTia/typhoeus that referenced this pull request Jan 4, 2019
As of lostisland/faraday#841, we've changed the exceptions namespace.
This change needs to be reflected on exceptions raised by this adapter.
This change is required in order to support Faraday 1.0, and should be fully backwards-compatible with older versions of Faraday.
Copy link
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some “looked through the code” comments.

Change is good!

@@ -1,5 +1,6 @@
RSpec.describe Faraday::Adapter::Typhoeus do
features :body_on_get, :parallel

it_behaves_like 'an adapter'
# Commenting until Typhoeus is updated to support v1.0
# it_behaves_like 'an adapter'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would an xit work?

expect { conn.get('ok') }.not_to raise_error
end

it 'raise Faraday::Error::ResourceNotFound for 404 responses' do
expect { conn.get('not-found') }.to raise_error(Faraday::Error::ResourceNotFound) do |ex|
it 'raise Faraday::ResourceNotFound for 400 responses' do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description could use the “raises” tense of that verb (throughout these cases). It was the wrong form before your changes, too.

@iMacTia
Copy link
Member Author

iMacTia commented Jan 4, 2019

@olleolleolle Thanks for pointing out both!
Unfortunately you can't use xit for shared examples, but I managed to get a similar result with a before hook 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants