mortada / fredapi

Python API for FRED (Federal Reserve Economic Data) and ALFRED (Archival FRED)
Apache License 2.0
930 stars 160 forks source link

Refactored tests to allow (and default) running tests without actual calls to FRED #5

Closed elmotec closed 9 years ago

elmotec commented 9 years ago

Hi there,

Great job on fredapi. I also happens to think that a tight integration with pandas makes a lot sense. I'd like to contribute.

The test suite required an internet connection and a FRED API key but that probably won't be available everywhere (I am thinking about travis-ci). So I refactored the tests that were there and added a variable (fake_fred_call) to tell whether to call FRED or not. fake_fred_call defaults to True.

I hope it makes sense. I'd like to integrate the updates API function as well, and integrate fredapi with travis-ci.

mortada commented 9 years ago

@elmotec thanks for the PR! The code changes related to switching to HTTPS look good. For the rest I'd like to spend some time to review/test first. Sorry I've been kind of swamped these days.

If you could make a separate PR for the HTTPS changes, I can merge that very soon and do a new release.

elmotec commented 9 years ago

@mortada, I created https://github.com/mortada/fredapi/pull/7.

Note that this PR may not integrate well with the master branch after we apply https://github.com/mortada/fredapi/pull/7 as my master and yours will have diverged on the same code lines. We'll see. Worst come to worst, I'll close this PR and create a new one.

mortada commented 9 years ago

@elmotec you won't have to close this PR and create a new one - you simply need to rebase against master and keep the same PR, you can look at the tutorial here: https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request

elmotec commented 9 years ago

@mortada, this pull-request has become messy and I apologize for it. It should be merge-able now that I re-based to your master. I will try to summarize the content here:

I've read a little about git, picking cherries and re-basing :smile: and I will create a separate branch for each feature going forward.

mortada commented 9 years ago

@elmotec great yes you've successfully rebased and it is definitely a good idea to create a separate branch for each PR

Another thing though, if you don't mind, is that you can squash these 13 commits down to one. As you noted the commits are now a bit messy. You can see a tutorial here: https://github.com/ginatrapani/todo.txt-android/wiki/Squash-All-Commits-Related-to-a-Single-Issue-into-a-Single-Commit

elmotec commented 9 years ago

@mortada, thanks for the pointers. I collapsed the 13 commits to one, and tried to relax the numpy and pandas version to those that were released a year ago. I could make the tests pass with pandas/numpy 0.15/1.7 (https://travis-ci.org/elmotec/fredapi/builds/71517465) without python 3.4 which had been released only a few months earlier.

elmotec commented 9 years ago

Hey @mortada, I made a few more changes to the tests to try to make as easy as possible to read.

mortada commented 9 years ago

@elmotec I noticed you squashed the commits down to 1 commit, thanks for that. This commit however is not linking to your github account though, so you wouldn't show up as a contributor when this PR is merged. You can take a look at this: https://help.github.com/articles/why-are-my-commits-linked-to-the-wrong-user/

elmotec commented 9 years ago

Thanks for pointing that out. I thought I had changed everything to elmotec, but apparently not. Give me a few days plz.

mortada commented 9 years ago

sure no worries :)

elmotec commented 9 years ago

Ok. I had my old email in the commit themselves, it seems. The latest one should point to @elmotec. Thanks for spotting it.

elmotec commented 9 years ago

Hey @mortada, would you like me to do something else with this PR?

mortada commented 9 years ago

sorry I've been really swamped these days, this looks good, merging now

mortada commented 9 years ago

thanks for the contribution!