Conversation
Generated by 🚫 Danger |
bguban
force-pushed
the
isin_endpoint
branch
2 times, most recently
from
3f376ed to
2e86e3e
Compare
dblock
left a comment
dblock
left a comment
There was a problem hiding this comment.
I think we should make it easier for users and have 2 APIs, asin and mapped_asin.
| module IEX | ||
| module Endpoints | ||
| module RefData | ||
| def isin_mapping(isins, options = {}) |
There was a problem hiding this comment.
The API is asin, why do we call it asin_mapping? Shouldn't it be asin?
There was a problem hiding this comment.
I called it as the documentation section was called (https://iexcloud.io/docs/api/#isin-mapping). As for me, it's a very controversial endpoint. It even doesn't return ISINs. It returns symbols that mutch to given ISINs.
Maybe it even would be better to call it ref_data_isin. What do you think?
| def isin_mapping(isins, options = {}) | ||
| response = post('ref-data/isin', { token: publishable_token, isin: isins }.merge(options)) | ||
|
|
||
| if response.is_a?(Hash) # mapped option was set |
There was a problem hiding this comment.
I really dislike these APIs that return two different things depending on what you pass into them. Why don't we make two methods: isin and mapped_isin?
There was a problem hiding this comment.
As you mentioned in another comment the client should not be too opinionated. Keeping it in mind, I think it's better to copy the behavior of IEX endpoint and have two different responses depending on options in the single method.
Again, if split the endpoint to two methods, the end-user can call isin endpoint passing there mapped: true option what causes an error while the response parsing. So it requires an end-user to know the client-specific method mapped_isin.
| expect(subject.last).to eq('exchange' => 'ETR', 'iex_id' => 'IEX_464D46474C312D52', 'region' => 'DE', 'symbol' => 'APC-GY') | ||
| end | ||
|
|
||
| context 'with mapped option', vcr: { cassette_name: 'ref-data/isin_mapped' } do |
There was a problem hiding this comment.
This context does not depend/inherit the parent one. Split them up here so each has its own section.
|
LGTM after the spec block change, feel free to merge on green then |
3 participants
No description provided.