-
Notifications
You must be signed in to change notification settings - Fork 52
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
added includeIntradayVolume to be passed to get_dataframe in TiingoCl… #673
base: master
Are you sure you want to change the base?
Conversation
A rough outline of the options offered by tiingo. Cell 14 needs a nosejob but the bones are good
Crypto Jupyter Notebook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this @datatalking , and for looking into including crypto documentation too. Once your API key has been reset / hidden, it will be a helpful resource for others learning the library.
I'd suggest putting the documentation notebook into a separate PR so that it can be reviewed separately from the new parameter.
|
||
#%% | ||
|
||
TIINGO_API_KEY = '8c35e8407d4da9c94e39bdf0dbc7913a899c2377' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you may want to invalidate this API key so other people can't reuse it / rewrite git history to not include this directly in your notebook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks hydrosquall, token is reset. Can you recommend a good tutorial on setting up passwords in environments if we are developing on one machine and testing on another? Keychain perhaps ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the python ecosystem, we use
I haven't used this personally, but the world of Python seems to have this as a similar project
Most CI providers (Jenkins, Gitlab, CircleCI etc) will offer a way to supply environment variables via a UI specific to their platform. Keychain is a good option if you're using Mac everywhere.
|
||
#%% | ||
|
||
!pip3 install os |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the dependencies for the demo notebooks can be included here rather than in inline cells:
https://github.com/hydrosquall/tiingo-python/blob/master/binder/requirements.txt
params = {"format": fmt, "resampleFreq": frequency} | ||
if includeIntradayVolume: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this will work, since includeIntradayVolume
isn't defined. Once that is added, could you include a unit test that will cover this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added includeIntradayVolume
as it was referenced in #462 which I figured includeIntradayVolume
had already been defined.
Do you have any recommendations for the location where should I add it?
If you can give me a few pointers on the location of the definition, I can probably work up a unittest PR for includeIntradayVolume
easily enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @datatalking , thanks for the patience
What I meant was that the current code doesn't work because this variable was never defined.
I think you would probably want to add includeIntradayVolume
as an additional parameter to the get_dataframe
method .
Lines 265 to 267 in 5142324
frequency="daily", | |
fmt="json", | |
): |
Including includeIntradayVolume in the get_dataframe method signature as
#462 proposes would work well, without it this “if statement” won’t be able
to run.
As for unit tests, this file is where we store the other tests for the
get_dataframe method.
https://github.com/hydrosquall/tiingo-python/blob/master/tests/test_tiingo_pandas.py
…On Tue, Aug 31, 2021 at 8:08 PM Andrew Schell ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In tiingo/api.py
<#673 (comment)>
:
> params = {"format": fmt, "resampleFreq": frequency}
+ if includeIntradayVolume:
I added includeIntradayVolume as it was referenced in #462
<#462> which I figured
includeIntradayVolume had already been defined.
Do you have any recommendations for the location where should I add it?
If you can give me a few pointers on the location of the definition, I can
probably work up a unittest PR for includeIntradayVolume easily enough.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#673 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACE2MM57IIFVV5JRX2L4UJ3T7VVGXANCNFSM5C2LXUBQ>
.
|
…ient
🎉 Hello there 🎉! Thanks for taking the time to contribute to
Tiingo-Python
!Summary of Changes
Checklist
HISTORY.rst
with an entry summarizing your change