dblock / iex-ruby-client

IEX Finance API Ruby Client
MIT License
119 stars 60 forks source link

Add InvalidOptionError Exception #8

Closed rodolfobandeira closed 6 years ago

rodolfobandeira commented 6 years ago

close #2

Raise an InvalidOption Exception if an invalid option is passed to Chart.get and IEX returns 400 ClientError

rodolfobandeira commented 6 years ago

@dblock I noticed that Rubocop is bitching about some rules that aren't actually failing on my local. What should we do? Create a custom local rubocop.yml?

dblock commented 6 years ago

For Robocop run rubocop -a ; rubocop --auto-gen-config.

dblock commented 6 years ago

Looks like IEX returns a 400 on all kinds of errors, such as this one. I think you should create a BadRequestError and parse the returned body instead using the error field as the message or "Unexpected error" when that's not available + exposing parsed error details in the exception.

rodolfobandeira commented 6 years ago

At first, I thought that since this exception is only available through the Chart.get, it would be enough to assume that any 400/ClientError would be a result of invalid parameters. What you said make sense. I'll review the logic and implement what you suggested. I'll update in here later this week. Thanks @dblock

rodolfobandeira commented 6 years ago

Hey @dblock, thanks for reviewing the code. I added two additional commits to address the points you suggested. Now we're filtering by that specific string "option" is not allowed. If it doesn't match that condition, we'll raise our own BadRequestError. The only problem now is figuring out how to write a propper RSpec to fake this edge case with "Bad Request". I found that adding no parameter gives an ArgumentError, and adding invalid data on valid parameters, returns an URI Exception.

Please let me know your thoughts, Cheers

cc: @jromanovs

rodolfobandeira commented 6 years ago

Hey @dblock, I agree with you. As long as it overrides the ugly Faraday exception, I believe it is something achieved. So I rolled-back the InvalidOptionError and left just the BadRequestError.

I'm gonna try working on some missing methods for the API to familiarize better with the project. Cheers

rodolfobandeira commented 6 years ago

Let's move this PR/thread to #9