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 support to fetch a single key stat #106

Merged
merged 5 commits into from
Aug 14, 2021

Conversation

agrberg
Copy link
Collaborator

@agrberg agrberg commented Aug 12, 2021

Alternatives I considered:

Keep the method signature the same and supplying the stat in the options hash

def key_stats(symbol, options = {})
  stat = options.delete(:stat)
  params = { token: publishable_token }.merge(options)
  url = "stock/#{symbol}/stats"
  return get("#{url}/#{stat}", params) if stat

  IEX::Resources::KeyStats.new(get(url, params))
  

Wrap the single stat in a IEX::Resources::KeyStats instance

def key_stats(symbol, stat = nil, options = {})
  params = { token: publishable_token }.merge(options)
  url = "stock/#{symbol}/stats"
  data = if stat
           { stat => get("#{url}/#{stat}", params) }
         else
           get(url, params)
         end

  IEX::Resources::KeyStats.new(data)

This would keep the return type the same and with nil being provided for any of the other stats. E.g.:

single_stat = client.key_stats('VTI', 'dividendYield')
single_stat.dividend_yield # 0.01271760965303361
single_stat.pe_ratio # nil

Let me know what you think!

@dblock
Copy link
Owner

dblock commented Aug 13, 2021

I definitely prefer the former because the latter is an API change, and we should only do this very carefully. Options is exactly designed for those ... optional options :)

Next, the result of this is a single value? Should we just add a key_stat method instead of having this one return two different types?

@agrberg
Copy link
Collaborator Author

agrberg commented Aug 13, 2021

🤔 that's a great question. There's no reason that because IEX's API groups a single stat as a special case of all basic stats that the client need to do the same.

I think having a key_stat endpoint solves all of the issues:

  • There is no API breaking change for a now mandatory stat client.key_stat(symbol, stat, options)
  • The return type of the existing method doesn't change and it's expected that key_stat returns the data in the type of the choosen the stat.
  • Simpler to read logic

context 'invalid stat', vcr: { cassette_name: 'key_stat/invalid_stat' } do
subject { client.key_stat(symbol, 'INVALID') }

it 'returns a hash of the API response' do
Copy link
Owner

Choose a reason for hiding this comment

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

This is a weird API. So if you specify something invalid it fails silently and returns the whole stats object? That's just bad design - I don't expect an API to succeed and return a different type of object. Shall we have the client raise a specialized exception in this case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't either 🤷 I originally wrote the test to raise an error but was surprised when the test failed because the request "succeeded." I'm in favor of straight forward APIs w/ little nuance so an exception would be good here.

CHANGELOG.md Outdated
@@ -1,5 +1,6 @@
### 1.4.2 (Next)
* [#105](https://github.com/dblock/iex-ruby-client/pull/105): Added support for fetching latest foreign exchange rates - [@mathu97](https://github.com/mathu97).
* [#106](https://github.com/dblock/iex-ruby-client/pull/106): Added support for fetching a single key stat in `key_stat(symbol, stat)` endpoint - [@agrberg](https://github.com/agrberg).
Copy link
Owner

Choose a reason for hiding this comment

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

Technically in -> with :)

@agrberg
Copy link
Collaborator Author

agrberg commented Aug 14, 2021

Rebased and updated ☝️

@agrberg agrberg requested a review from dblock August 14, 2021 18:17
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.

One last question. This looks good.

module KeyStat
def key_stat(symbol, stat, options = {})
result = get("stock/#{symbol}/stats/#{stat}", { token: publishable_token }.merge(options))
raise IEX::Errors::StatNotFoundError.new(stat, result) if result.is_a?(Hash)
Copy link
Owner

Choose a reason for hiding this comment

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

Should this be unless result.is_a?(Number) or similar?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wasn't able to find an exhaustive list of types IEX currently returns. From experience I know it can return numbers, strings (for strings and dates), and booleans. Testing for one type I know indicates an invalid stat is simpler and more permissive of future changes with IEX's API. 😅 For all I know IEX will return some stat as a tuple encoded as an array and someone would have a bad day debugging because the requested stat is valid.

@dblock dblock merged commit e11fb64 into dblock:master Aug 14, 2021
@dblock
Copy link
Owner

dblock commented Aug 14, 2021

Merged. Thanks!

@agrberg agrberg deleted the ar/add_single_key_stat branch August 14, 2021 21:46
@agrberg
Copy link
Collaborator Author

agrberg commented Aug 14, 2021

happy to give back 💪

@dblock
Copy link
Owner

dblock commented Aug 15, 2021

Maybe time to cut a release?

@agrberg
Copy link
Collaborator Author

agrberg commented Aug 15, 2021

Good idea. Just launched v 1.5.0 w/ the new endpoint 🎉

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