Skip to content

Conversation

@dcwtx
Copy link
Contributor

@dcwtx dcwtx commented Apr 13, 2018

Resolves #82 and affected vcr files have been regenerated. Note in order to regenerate the test fixtures I had to set the api key through the environment variable to my personal api key, which ended up putting my api key into the .yaml files. I had to then edit them by hand and replace my api key with 0000000000000000000000000000000000000000 before committing them. After doing that I was able to set the api key to 0*30 and rerun py.test, which was successful, since this key had been stored in the .yaml files. It might be helpful to put something in the CONTRIBUTING.rst file regarding this.

@dcwtx
Copy link
Contributor Author

dcwtx commented Apr 13, 2018

I am going to add a test of the weekly data later today.

@codecov
Copy link

codecov bot commented Apr 13, 2018

Codecov Report

Merging #84 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@         Coverage Diff          @@
##           master   #84   +/-   ##
====================================
  Coverage      95%   95%           
====================================
  Files           4     4           
  Lines         120   120           
====================================
  Hits          114   114           
  Misses          6     6
Impacted Files Coverage Δ
tiingo/api.py 94.62% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d989cff...e7315db. Read the comment docs.

@hydrosquall
Copy link
Owner

hydrosquall commented Apr 14, 2018

@dcwtx I just merged a PR that upgrades the library that complains in python 3.3- can you try rebasing your branch against master to see if that fixes it?

I've also written up an issue to implement your suggestions about scrubbing the API key + automate part of the process for the benefit of future contributors in #86 :).

Update: upgrading the library broke master, so I've decided to just drop support for 3.3 altogether. Rebasing this branch against master should fix it.

Minor blip aside, this PR looks great! Please feel free to add yourself to the AUTHORS.rst if you'd like.

@dcwtx
Copy link
Contributor Author

dcwtx commented Apr 16, 2018

@hydrosquall, I submitted a request to GitHub to erase that old pull request #83 that included the api key in the diffs. They need you to approve the deletion before they do it. You can choose one of two ways to approve; email them at support@github.com, or send me an email and I'll include you on the email on this matter. Thanks -

@hydrosquall hydrosquall merged commit eaf1a8c into hydrosquall:master Apr 21, 2018
@hydrosquall hydrosquall self-requested a review May 1, 2018 02:58
@dcwtx dcwtx deleted the issue82 branch May 10, 2018 03:30
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.

Incorrect resample argument for rest api

2 participants