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 ClientError Exception #9

Merged
merged 4 commits into from Jul 27, 2018

Conversation

rodolfobandeira
Copy link
Collaborator

close #2

Copy link
Owner

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Better, but doesn't work (see bug below).

The Faraday::ClientError raised is wrapping all errors 400-600, so not just HTTP 400 Bad Request (. See https://github.com/lostisland/faraday/blob/master/lib/faraday/response/raise_error.rb#L12). I suggest renaming BadRequest to ClientError to match.

README.md Outdated
@@ -146,6 +146,14 @@ See [#chart](https://iextrading.com/developer/docs/#chart) for detailed document

If a symbol cannot be found an [IEX::Errors::SymbolNotFound](lib/iex/errors/symbol_not_found_error.rb) exception is raised.

### BadRequest

All 400 bad request responses from IEX result in a BadRequest exception
Copy link
Owner

Choose a reason for hiding this comment

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

Nitpick: change BadRequest to IEX::Errors::BadRequest as above and link it to the .rb file.

@@ -21,6 +21,8 @@ def self.get(symbol, range = nil, options = {})
end
rescue Faraday::ResourceNotFound => e
raise IEX::Errors::SymbolNotFoundError.new(symbol, e.response[:body])
rescue Faraday::ClientError => e
raise IEX::Errors::BadRequestError, e.response[:body]['error']
Copy link
Owner

Choose a reason for hiding this comment

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

There's a bug here. You're passing in the text of the error, but the exception itself takes a response object. This should be e.response and the exception should be doing super e.response[:body]['error'].

end

it 'fails with BadRequestError' do
expect { subject }.to raise_error IEX::Errors::BadRequestError
Copy link
Owner

Choose a reason for hiding this comment

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

You should be checking that the exception caught has a .response object and that its error message is the extracted message from the body.

@rodolfobandeira
Copy link
Collaborator Author

Hey @dblock. Renamed to ClientError.

@rodolfobandeira rodolfobandeira changed the title Add BadRequestError Add ClientError Exception Jul 26, 2018
Copy link
Owner

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Almost, see requests. Thanks for hanging in here.

README.md Outdated
@@ -146,6 +146,14 @@ See [#chart](https://iextrading.com/developer/docs/#chart) for detailed document

If a symbol cannot be found an [IEX::Errors::SymbolNotFound](lib/iex/errors/symbol_not_found_error.rb) exception is raised.

### ClientError

All 400-600 bad request responses from IEX result in a [IEX::Errors::Client](lib/iex/errors/client_error.rb) exception.
Copy link
Owner

Choose a reason for hiding this comment

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

Type: ClientError, not Client.

README.md Outdated
@@ -146,6 +146,14 @@ See [#chart](https://iextrading.com/developer/docs/#chart) for detailed document

If a symbol cannot be found an [IEX::Errors::SymbolNotFound](lib/iex/errors/symbol_not_found_error.rb) exception is raised.

### ClientError

All 400-600 bad request responses from IEX result in a [IEX::Errors::Client](lib/iex/errors/client_error.rb) exception.
Copy link
Owner

Choose a reason for hiding this comment

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

They are not "bad request responses", say "All errors that return HTTP codes 400-600 result in ..."

@@ -1,3 +1,3 @@
module IEX
VERSION = '0.3.4'.freeze
VERSION = '0.3.5'.freeze
Copy link
Owner

Choose a reason for hiding this comment

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

Undo this.

@@ -72,4 +77,14 @@
expect { subject }.to raise_error IEX::Errors::SymbolNotFoundError, 'Symbol INVALID Not Found'
end
end

context 'with client error', vcr: { cassette_name: 'chart/badRequest' } do
Copy link
Owner

Choose a reason for hiding this comment

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

Lets rename the badRequest file here to bad_option to match the name of the bad option.

@rodolfobandeira
Copy link
Collaborator Author

@dblock All good! Thank YOU!

@dblock dblock merged commit afd956d into dblock:master Jul 27, 2018
@dblock
Copy link
Owner

dblock commented Jul 27, 2018

Merged.

@rodolfobandeira rodolfobandeira deleted the add-bad-request-error branch July 30, 2018 01:10
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.

Raise specific exceptions on 400
2 participants