Skip to content

Implement Google options functionality #183

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 5, 2016
Merged

Implement Google options functionality #183

merged 1 commit into from
Mar 5, 2016

Conversation

gliptak
Copy link
Contributor

@gliptak gliptak commented Feb 28, 2016

Please review. Thanks

@gliptak gliptak mentioned this pull request Feb 28, 2016
Closed
sinhrks
sinhrks reviewed Feb 28, 2016
View reviewed changes
pandas_datareader/google/options.py
def _process_rows(self, l, rows_list, now, index, expiry, type, underlying_price):
for row in l:
dict = {}
try:
Copy link
Member

Choose a reason for hiding this comment

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

I prefer loops over keys to avoid repeat.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved inside _process_rows()

This was referenced Feb 28, 2016
Closed
Closed
sinhrks
sinhrks reviewed Feb 28, 2016
View reviewed changes
pandas_datareader/google/options.py
for key, type in [['calls', 'call'], ['puts', 'put']]:
for row in jd[key]:
d = {}
try:
Copy link
Member

Choose a reason for hiding this comment

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

Use loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you offer a snippet on how to change this?

@sinhrks
Copy link
Member

sinhrks commented Feb 28, 2016

Thanks for this. Maybe we should fix / refactor yahoo options at some point.

femtotrader
femtotrader reviewed Feb 28, 2016
View reviewed changes
pandas_datareader/google/options.py
return df.sort_index()

def _process_rows(self, jd, rows_list, now, index, expiry):
for key, type in [['calls', 'call'], ['puts', 'put']]:
Copy link
Contributor

Choose a reason for hiding this comment

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

typ not type because of shadowing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@gliptak
Copy link
Contributor Author

gliptak commented Feb 28, 2016

@sinhrks Would an issue be the best forum to discuss refactoring?

sinhrks
sinhrks reviewed Feb 29, 2016
View reviewed changes
pandas_datareader/google/options.py

The class has the following methods:
get_options_data(month, year, expiry)
get_call_data(month, year, expiry)
Copy link
Member

Choose a reason for hiding this comment

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

Following methods are not supported, correct? Pls update docstring and example to meet the current spec (or raise NotImplementedError).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now I'm raising NotImplementedError.

@sinhrks
Copy link
Member

sinhrks commented Mar 1, 2016

Thanks for the update. Can you also update docs (remote_data.rst and whatsnew)?

@gliptak
Copy link
Contributor Author

gliptak commented Mar 1, 2016

Done.

@gliptak
Copy link
Contributor Author

gliptak commented Mar 3, 2016

The failed tests are unrelated

FAIL: test_dtypes (pandas_datareader.tests.test_google.TestGoogle)
ERROR: test_get_tourism (pandas_datareader.tests.test_oecd.TestOECD)
ERROR: test_get_un_den (pandas_datareader.tests.test_oecd.TestOECD)
ERROR: test_oecd_invalid_symbol (pandas_datareader.tests.test_oecd.TestOECD)

@sinhrks
Copy link
Member

sinhrks commented Mar 3, 2016

I've retriggered tests.

@gliptak
Copy link
Contributor Author

gliptak commented Mar 3, 2016

Would you like to see some other updates for this? Thanks

@sinhrks
Copy link
Member

sinhrks commented Mar 5, 2016

Thanks, looks good to me. Could you squash to single commit?

@femtotrader ?

@gliptak
Copy link
Contributor Author

gliptak commented Mar 5, 2016

Squashed.

@femtotrader
Copy link
Contributor

It sounds good to me also. @sinhrks you can merge.

sinhrks added a commit that referenced this pull request Mar 5, 2016
@sinhrks
Merge pull request #183 from gliptak/googleoptions
f20133d 
Implement Google options functionality
@sinhrks sinhrks merged commit f20133d into pydata:master Mar 5, 2016
@sinhrks
Copy link
Member

sinhrks commented Mar 5, 2016

@gliptak Thanks!

@gliptak gliptak deleted the googleoptions branch July 9, 2016 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gliptak @sinhrks @femtotrader