-
-
Notifications
You must be signed in to change notification settings - Fork 60
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
Remove default range and use iextrading default range 1m instead in IEX::Api::Dividends
#39
Conversation
CHANGELOG.md
Outdated
@@ -1,5 +1,6 @@ | |||
### 0.4.4 (next) | |||
|
|||
* [#39](https://github.com/dblock/iex-ruby-client/pull/39): Fix typo: remove useless range - [@ildarkayumov](https://github.com/ildarkayumov) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually you've fixed a bug, not a typo, so this should describe what was fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want me to change commit and changelog?
thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, my initial comment was wrong. Just describe the actual fix. Maybe README also needs to say that the default range is '6m'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, better not to overwrite default range because iex api use 1m
as default
https://api.iextrading.com/1.0/stock/aapl/dividends
So we can simply use range
IEX::Api.default_connection [
symbol,
'dividends',
range
].compact.join('/')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that makes a lot of sense, I prefer that. Make the changes! Thanks.
Nice catch, this is a bug. Get this to 💚 and thank you! |
8de0dc7
to
d02ce0f
Compare
lib/iex/api/dividends.rb
Outdated
@@ -9,8 +9,7 @@ def self.connection(symbol, range = nil) | |||
IEX::Api.default_connection [ | |||
symbol, | |||
'dividends', | |||
range ? '6m' : nil, | |||
range | |||
range ? range : '6m' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be range || '6m'
CHANGELOG.md
Outdated
@@ -1,5 +1,6 @@ | |||
### 0.4.4 (next) | |||
|
|||
* [#39](https://github.com/dblock/iex-ruby-client/pull/39): Fix: remove useless range in dividends api - [@ildarkayumov](https://github.com/ildarkayumov). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually fixes the default range, doesn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It fixes default range and removes duplicated range use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets say that
d02ce0f
to
cad4b00
Compare
IEX::Api::Dividends
Sorry for the delay, merged. |
It looks like there is a typo when using range option.
for example
generates
"AAPL/dividends/6m/5y"
instead of
"AAPL/dividends/5y"
as described in docs - https://iextrading.com/developer/docs/#dividends