option_chain() return underlying data that comes with the options data

[DanielGoldfarb]

options_chain() return underlying data that comes with the options data. this is helpful to ensure that underlying bid,ask,trade and options bid,ask,trade are in sync, plus avoids an additional call since the data comes from the options chain request anyway.

[ValueRaider]

I don't use options so can't review. Maybe these can? @thezenvan @GF-Huang @yukunchen113 @gracekk

[DanielGoldfarb]

I am happy for anyone to review this; note that while someone who knows options may well understand some of the motivation behind this, in fact anyone can review the code and note the following:

1. I have implemented this in such a way that it should have no effect on anyone who is not interested in this data: The pre-existing parts of the namedtuple returned by method option_chain() (i.e. Options.call and Options.put) remain unchanged; there is now an additional attribute Options.underlying which contains the dict of underlying information that always gets returned with an options request().
2. I have tweaked the output from the internal method _download_options() to always return a dict. Previously, when data was available it would return a dict, otherwise it would return an empty list or None, depending which code path was taken.

That's the extent of this code change. Thanks!

[ricardoprins]

LGTM.

It gives some more useful info in a non-clunky way. I'd only note the change in return type in _download_options(), but I don't think it yields any negative impact.

[ValueRaider]

I'd only note the change in return type in _download_options()

Why? I misread, thought you were suggesting changing type.

Otherwise, I've quickly looked at code, is a simple change.

[DanielGoldfarb]

2. I have tweaked the output from the internal method _download_options() to always return a dict. Previously, when data was available it would return a dict, otherwise it would return an empty list or None, depending which code path was taken.

I realize now that I explained what I did but not why. _download_options() was inconsistent in its return types, as mentioned in the quote above. It is an internal function to the class, and as far as I can tell it is called in only the one location. In either case, if it has no data to return, then an exception will be thrown when option_chain() attempts index into the returned value options, but at least now the returned type will be the same in all cases, and so the exception will be the same in all cases.

Alternatively I can now take this a step further. since the returned type will always be a dict, I can modify option_chain to handle the empty dict by returning empty dataframes instead of raising a KeyError when 'calls' is not in the dict.

At any rate, I thought it best for _download_options() to be consistent in the type that it returned.

[ValueRaider]

@DanielGoldfarb Up to you. I'm OK merging this as is.

[DanielGoldfarb]

I would like to merge this as is, since this does not change the behavior of an "client facing" methods, and I would prefer to leave it at that. Thanks for reviewing!

[ricardoprins]

2. I have tweaked the output from the internal method _download_options() to always return a dict. Previously, when data was available it would return a dict, otherwise it would return an empty list or None, depending which code path was taken.

I realize now that I explained what I did but not why. _download_options() was inconsistent in its return types, as mentioned in the quote above. It is an internal function to the class, and as far as I can tell it is called in only the one location. In either case, if it has no data to return, then an exception will be thrown when option_chain() attempts index into the returned value options, but at least now the returned type will be the same in all cases, and so the exception will be the same in all cases.

Alternatively I can now take this a step further. since the returned type will always be a dict, I can modify option_chain to handle the empty dict by returning empty dataframes instead of raising a KeyError when 'calls' is not in the dict.

At any rate, I thought it best for _download_options() to be consistent in the type that it returned.

I agree 100% with your reasoning here.