Skip to content

One file per datareader #59

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

Closed
wants to merge 12 commits into from
Closed

One file per datareader #59

wants to merge 12 commits into from

Conversation

femtotrader
Copy link
Contributor

Should fix #58

@0x0L 0x0L mentioned this pull request Aug 22, 2015
@femtotrader
Copy link
Contributor Author

@@ -84,1225 +69,11 @@ def DataReader(name, data_source=None, start=None, end=None,
retry_count=retry_count, pause=pause)
elif data_source == "google":
return get_data_google(symbols=name, start=start, end=end,
adjust_price=False, chunksize=25,
retry_count=retry_count, pause=pause)
chunksize=25, retry_count=retry_count, pause=pause)
Copy link
Member

Choose a reason for hiding this comment

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

Why is adjust_price removed?

@davidastephens
Copy link
Member

Do we need the datareaders package? Can it just be pandas_datareader/google/..?

@femtotrader
Copy link
Contributor Author

I created the datareaders package because I didn't like the fact of having for example fred.py famafrench.py, google and yahoo in the same directory than date_chunks.py, shared.py and url_request.py

I can remove it if you think that's more understandable and put date_chunks.py, shared.py and url_request.py in a commons package.

adjust_price is removed because Google don't provide it (only Yahoo)
That's the reason why

if ret_index:
    hist_data['Ret_Index'] = _calc_return_index(hist_data['Adj Close'])
if adjust_price:
    hist_data = _adjust_prices(hist_data)

is in get_data_yahoo

@femtotrader
Copy link
Contributor Author

@davidastephens I have update accordingly.

but I wonder if commons is a great package name.

It could be :

  • commons
  • common
  • shared

@femtotrader
Copy link
Contributor Author

I also think that some cleanup is / will be necessary.

For example,

  • in famafrench.py
    • _FAMAFRENCH_URL should be named _URL
    • get_data_famafrench should be name only _get_data (but we still need to expose data.get_data_famafrench for compatibility reason)

most files should have such cleanup

options.py should also be improved because it's in yahoo so there is no need for data_source

we should have an other options.py at same level than data.py with one function Option to perform dispatching between vendors. (for now only yahoo is supported)

@femtotrader
Copy link
Contributor Author

Options is now also a function in data.py which is able to dispatch per data vendor. For now there is only one data "vendor" for options : Yahoo.

@davidastephens
Copy link
Member

I think for now, given there is only 2 things in the common package, we can just have it as common.py in the base package.

@davidastephens
Copy link
Member

I think we should merge the package structure changes and do the clean-up and option dispach in 2 separate pulls.

Can you rebase against master and squash to one commit (except the option dispatch and clean-up?).

Thanks for all the work on this.

@femtotrader
Copy link
Contributor Author

Hum... that will be quite difficult for me. I'm not very good with git. Maybe I should create an other branch ?

@davidastephens
Copy link
Member

There is a good tutorial here:
https://github.com/pydata/pandas/wiki/Using-Git

Here are the steps you need to do:

#add the pandas remote repository to your fork and name it upstream

git remote add upstream git://github.com/pydata/pandas-datareader

#fetch and then rebase interactively to squash, reword, fix, otherwise change some commits

git fetch upstream
git rebase --interactive upstream/master

In the text file you get from rebasing, just remove the Option and cleanup commits (delete the lines) and change the rest of them from pick to squash (except the first one). That squashes them all into the first commit.

@femtotrader
Copy link
Contributor Author

Closing this PR is favor of #68

@femtotrader
Copy link
Contributor Author

Sorry

@femtotrader femtotrader mentioned this pull request Aug 23, 2015
@femtotrader femtotrader deleted the one_file_per_datareader branch September 26, 2015 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

One file per datareader
3 participants