-
Notifications
You must be signed in to change notification settings - Fork 681
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 tiingo csv response serialisation options #822
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #822 +/- ##
==========================================
- Coverage 64.67% 63.27% -1.40%
==========================================
Files 62 62
Lines 2848 2775 -73
Branches 305 306 +1
==========================================
- Hits 1842 1756 -86
- Misses 932 945 +13
Partials 74 74 ☔ View full report in Codecov by Sentry. |
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.
Mostly looks good. Hard par tis testing it given the API key.
pandas_datareader/tiingo.py
Outdated
If no value is provided, defaults to 5min. The minimum value is "1min". | ||
Units in minutes (min) and hours (hour) are accepted. | ||
Re-sample frequency. Format is #min/hour; e.g. "15min" or "4hour". | ||
If no value is provided, defaults to 5min. The minimum value is\ |
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.
Can you remove to \ and fix spacing.
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.
Done.
pandas_datareader/tiingo.py
Outdated
"1min". | ||
Units in minutes (min) and hours (hour) are accepted. | ||
response_format : str, default 'json' | ||
Format of response data returned by the underlying Tiingo REST API. |
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.
Is this accurate? Perhaps something like: "The format to use in the request tot he TIINGO API. "csv" results in smaller payload, less bandwidth, and may result in smaller charges for accessing the TIINGO API"
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.
Sounds good - added wording along those lines. This would affect bandwidth limits and implicitly charges.
pandas_datareader/tiingo.py
Outdated
@@ -83,12 +90,15 @@ def __init__( | |||
"environmental variable TIINGO_API_KEY." | |||
) | |||
self.api_key = api_key | |||
self.response_format = ( |
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.
You should raise ValueError if not one of the expected values.
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.
Good point - done!
Regarding testing. The API has free plan - it would be possible to setup dedicated key for automated tests ( https://api.tiingo.com/about/pricing ) |
Hi, Just a quick ping to check whether I am missing anything for this PR to get it merged for a release? |
git diff upstream/master -u -- "*.py" | flake8 --diff
black --check pandas_datareader