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

Issue 28 implement ipo calendar #33

Closed
wants to merge 8 commits into from
Closed

Issue 28 implement ipo calendar #33

wants to merge 8 commits into from

Conversation

UNISERVO1
Copy link

PR to Implement IPO Calendar #28

@UNISERVO1
Copy link
Author

@rodolfobandeira, @dblock: Please review and let me know what additional modifications/refactors are needed.

CHANGELOG.md Outdated
@@ -1,6 +1,7 @@
### 0.4.4 (next)

* [#32](https://github.com/dblock/iex-ruby-client/pull/32): Add `IEX::Resource::Sectors` - [@gil27](https://github.com/gil27).
* [#33](https://github.com/dblock/iex-ruby-client/pull/33): Add `IEX::Resource::Sectors` - [@marchyoung](https://github.com/marchyoung).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please fix the typo. I guess it should be something like: IEX::Resources::IPOCalendar.today/upcoming ?


### Get IPO Calendar

Fetches a list of IPOs scheduled for the current and next month. Use `today` to get a list of today's IPOs and `upcoming` to get a list of upcoming IPOs. Each scheduled IPO listing is schematically split into separate lists of comprehensive IPO information in `raw_data` with a corresponding abridged summary in `view_data`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

You did a great job on this PR @marchyoung !

I understand your point adding these two options. Although when I first read raw_data, I was expecting to see indeed the raw data coming from the API, but the data is being changed using the lambdas on the IPO class. That said, in favor for backwards compatibility, I would move the view_data/raw_data from the response in a different way into one additional method only that returns the data you want.

IEX::Resources::IPOCalendar.today
IEX::Resources::IPOCalendar.today_summary
IEX::Resources::IPOCalendar.upcoming
IEX::Resources::IPOCalendar.upcoming_summary

So, this way, we assume that calling .today would return raw_data by default. If a light version of the returned data is requested, then calling .today_summary would do what the view_data is currently returning.

ipos_today = IEX::Resources::IPOCalendar.today
ipos_today.first.market # => 'NASDAQ Global Select'
ipos_today.first.ceo # => 'Stephane Kasriel'

@dblock Any thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

@rodolfobandeira , @dblock : Thanks for the feedback! I'd be more than happy to make view_data/raw_data return the "JSON-to-Hashie" conversion values only and move the lambda-based conversion logic to new today/upcoming and today_summary/upcoming_summary methods.

Copy link
Owner

Choose a reason for hiding this comment

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

Right on @rodolfobandeira, we want a collection of first class objects here.

@rodolfobandeira
Copy link
Collaborator

Hey @marchyoung,

Any update on this? if you're busy, I can try helping out by making a PR to your fork. Please let us know, thanks!

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants