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

backwards compatible refactoring of io/data options functionality #3817

Closed
gliptak opened this issue Jun 9, 2013 · 15 comments
Closed

backwards compatible refactoring of io/data options functionality #3817

gliptak opened this issue Jun 9, 2013 · 15 comments
Labels
IO Data IO issues that don't fit into a more specific label Refactor Internal refactoring of code
Milestone

Comments

@gliptak
Copy link
Contributor

gliptak commented Jun 9, 2013

I would like to propose an interface breaking refactoring to io/data options.

Here are the proposed major changes:

  • replace month/year with datetime parameter (the current implementation does not support weekly options)
  • add a call to list available expirations (I'm unsure how that is supported with Yahoo Finance data)
  • add data_source to all functions (where applicable) and refactor to _yahoo functions

This is in preparation to hooking up Google Finance as an options data source. (#3822)

Please comment. Thanks

@jreback
Copy link
Contributor

jreback commented Jun 9, 2013

not that I am against breaking an API, but seems the first point can be done in a backwards compat manner (at least for 1 release), but accepting the old parms and converting to your new one?

@gliptak
Copy link
Contributor Author

gliptak commented Jun 9, 2013

@jreback I'm certainly open to try to work this in backward compatible fashion. Could you please also comment on #3814 (as I'm modifying the same files)? Thanks

@jreback
Copy link
Contributor

jreback commented Jun 9, 2013

can u put up the new call signature?

@gliptak
Copy link
Contributor Author

gliptak commented Jun 9, 2013

@jreback please see #3822
@spencerlyon2 please offer your comments

I would like to see the semantics of these functions reviewed. Defaulting to current month is unclear for instruments with weekly options. Maybe an expiry date could always be required instead?

In addition i would like to remove get_forward_data() as it can be easily replicated by multiple calls into the other API points.

@sglyon
Copy link
Contributor

sglyon commented Jun 10, 2013

@gliptak, most of these changes look great. I do, however, appriciate @jreback's comment about not breaking the API for the months.

Also, I realize that get_forward_data() can easily be replicated with other calls (that's what its implementation does), but do we feel it is a bad thing to have a utility method around that makes a specific task easier? If so, feel free to get rid of it. Otherwise, I know that I call that method frequently.

@gliptak
Copy link
Contributor Author

gliptak commented Jun 11, 2013

@spencerlyon2 Spencer, yes let's keep year/month around for a release and the changes I proposed facilitate this.

As for get_forward_data(), the least I would like to try to refactor it to use the other calls. More generally, probably the others are also to call some sort of _utility_function() parsing out the data from the Yahoo page.

As I discussed above, I would like to modify the semantics of all options methods to support weekly expiries, which means sending in an actual expiry date (year/month/day) acquired from a list of expiries function call and this is where I'm a bit fuzzy on what get_forward_data() should be returning ... When it is the 'nearest' expiry, is that the nearest weekly or monthly? And how about if I call this function on Sunday and the just the current expiry still shows on Yahoo?

I do understand that Yahoo displays all expiries in a given month on a single page (I'm actually not sure how to generate the list of all expiries from the Yahoo data ...), while Google have them separated based on all expiries.

Please offer your thoughts. Thanks

@jtratner
Copy link
Contributor

@jreback quick question here - how does interface-breaking refactoring work with version numbers? I'm surprised this would be put in 0.11.1 if it actually breaks the previous API.

@jreback
Copy link
Contributor

jreback commented Jun 20, 2013

this appears to be back-compat; introducing a new API is ok as long as it accepts the old (or we deprecate and usually wait a version)

@jtratner
Copy link
Contributor

@jreback ah, okay, sorry I read "interface-breaking" as "backwards-incompatible"

@gliptak
Copy link
Contributor Author

gliptak commented Jun 20, 2013

@jtratner started out as interface breaking, but morphed into backward compatible based on the advice given

@jreback
Copy link
Contributor

jreback commented Jun 20, 2013

@gliptak in 0.12 if you want to break the API, pls open a new issue (as a reminder), see Note to Selves

(I am not sure you actuallly need to do that ....but think about it)

@jreback
Copy link
Contributor

jreback commented Jun 20, 2013

@gliptak this is not hooked to travis for some reason...?

@gliptak
Copy link
Contributor Author

gliptak commented Jun 20, 2013

@jreback what isn't hooked to Travis?

@jreback
Copy link
Contributor

jreback commented Jun 20, 2013

sorry thought was a pr for a sec :)

@jreback
Copy link
Contributor

jreback commented Jun 22, 2013

closed by #3822

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IO Data IO issues that don't fit into a more specific label Refactor Internal refactoring of code
Projects
None yet
Development

No branches or pull requests

4 participants