Add type cast for ref_data when parameter is string #70
Conversation
spec/iex/endpoints/ref_data_spec.rb
Outdated
| subject { client.ref_data_isin('US0378331005') } | ||
|
|
||
| it 'converts ISIN from string to Array and do not errors out' do | ||
| expect { subject }.not_to raise_error |
There was a problem hiding this comment.
This test doesn't test that the API actually works, aka that it returns the ISIN for the value you provide. Use an existing VCR that asks for 1 array item, aka we want 2 tests that it works the same way for "US0378331005" and for ["US0378331005"].
There was a problem hiding this comment.
Also probably want to highlight in README both examples (you can pass 1 value or an array of values).
There was a problem hiding this comment.
Good point. Please check the latest commit
There was a problem hiding this comment.
I still have an issue with expect { subject }.not_to raise_error, it's a test that says "make sure it doesn't throw an exception", which is weird. We expect all our calls not to throw errors ;) I'd remove it and make sure the values returned are actually correct (just like all tests).
There was a problem hiding this comment.
Also noticed that we have a nested block with different VCRs, the whole spec should look like this:
describe '#ref_data_isin' do
context 'with one asin', vcr: { cassette_name: 'ref-data/isin' }
context 'with multiple asins', vcr: ...
context 'with invalid asins', vcr: ...
endThere was a problem hiding this comment.
@dblock done. What do you think now?
README.md
Outdated
| @@ -381,6 +381,9 @@ Converts ISIN to IEX Cloud symbols. | |||
| ```ruby | |||
| symbols = client.ref_data_isin(['US0378331005']) | |||
|
|
|||
| # You can also pass single elements as string: | |||
There was a problem hiding this comment.
I am going to nitpick. It's not clear that you can pass an array of ISINs here because the sample has an array of 1.
I think you want both examples and not a comment.
Convert ISIN to IEX Cloud symbols.
example with 1
The API also lets you convert multiple ISINs to IEX Cloud symbols.
example with many
|
Rebase, update my nitpicks and let's merge this? |
rodolfobandeira
force-pushed
the
add-type-safe-for-ref-data-endpoint
branch
from
6c6a411
to
56be92d
Compare
|
@dblock Apologizes for the long time I had left this PR hanging there. Cheers |
CHANGELOG.md
Outdated
| @@ -9,6 +10,7 @@ | |||
| * [#71](https://github.com/dblock/iex-ruby-client/pull/71): Added `symbols` resource - [@ryosuke-endo](https://github.com/ryosuke-endo). | |||
| * [#69](https://github.com/dblock/iex-ruby-client/pull/69): Fixed `ref_data_isin` request - [@bguban](https://github.com/bguban). | |||
| * [#72](https://github.com/dblock/iex-ruby-client/pull/72): Cache `Faraday::Connection` for persistent adapters - [@dblock](https://github.com/dblock). | |||
| * [#69](https://github.com/dblock/iex-ruby-client/pull/69): Fixed ref_data_isin request - [@bguban](https://github.com/bguban). | |||
There was a problem hiding this comment.
Good catch. Removed
CHANGELOG.md
Outdated
| @@ -1,6 +1,7 @@ | |||
| ### 1.2.1 (Next) | |||
|
|
|||
| * [#82](https://github.com/dblock/iex-ruby-client/pull/82): Added `config.referer` to set HTTP `Referer` header. This enables IEX's "Manage domains" domain locking for tokens - [@agrberg](https://github.com/agrberg). | |||
| * [#70](https://github.com/dblock/iex-ruby-client/pull/70): Add type safe cast when ref_data_isin parameter is string - [@rodolfobandeira](https://github.com/rodolfobandeira). | |||
There was a problem hiding this comment.
maybe 'Added support for ref_data_isin taking a single String parameter", it's unclear what a type safe cast means here
There was a problem hiding this comment.
Totally agree.
Done
rodolfobandeira
added
hacktoberfest-accepted
and removed
hacktoberfest
Follow up on your comment @dblock