Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for tiingo crypto endpoints to TiingoClient #340

Merged
merged 9 commits into from
Oct 17, 2019

Conversation

n1rna
Copy link
Contributor

@n1rna n1rna commented Oct 12, 2019

馃帀 Hello there 馃帀! Thanks for taking the time to contribute to Tiingo-Python!

Summary of Changes

Checklist

  • [*] Code follows the style guidelines of this project
  • [*] Added tests for new functionality
  • [] Update HISTORY.rst with an entry summarizing your change
  • Tag a project maintainer

Copy link
Owner

@hydrosquall hydrosquall left a comment

Choose a reason for hiding this comment

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

This is a great start @n1rna ! Thank you for taking the time to contribute, I look forward to bringing in your changes to the next release!

tiingo/api.py Show resolved Hide resolved
from tiingo import TiingoClient


# CRYPTO ENDPOINTS
Copy link
Owner

Choose a reason for hiding this comment

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

These test cases are a great start. Would it be possible to take a pass at writing the test in a way that we add a new fixture to this folder https://github.com/hydrosquall/tiingo-python/tree/master/tests/fixtures? This way, people can see a sample of what the returned data is meant to look like.

Feel free to let me know if you have any questions about how to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, will add the fixtures ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a problem with getting responses from api.tiingo.com! I think my token is rate limited, but other than that, when I make an API call to crypt/top/ price and set a endDate querystring parameter, I get a 500 server error. I don't know it is raising an exception in tiingo's webserver or not.

Copy link

Choose a reason for hiding this comment

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

Thank you for the great work @n1rna! Please make sure to reach out to me get a free dev account!

You helped us catch an error that was related to older top of book data. We deprecated the "endDate" parameter for top-of-book since nobody seemed to use it and we plan to make order book tick data available shortly. This should help users reconstruct historical top-of-book using the tick data and promote transparency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!
So do I need to remove endDate parameter from the API client and also the test cases in this PR?

tiingo/api.py Show resolved Hide resolved
tiingo/api.py Outdated Show resolved Hide resolved
Copy link
Owner

@hydrosquall hydrosquall left a comment

Choose a reason for hiding this comment

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

This is almost there! Really neat to see how work on this client caught a backend error, thanks for the thorough bug report.

The main suggested change is to deprecate the parameter that was removed as a result of your bug report, the other comments are some optional curiosities I have about the fixtures.yaml.

Additionally, feel free to add yourself to the Authors.rst file if you'd like!

https://github.com/hydrosquall/tiingo-python/blob/master/AUTHORS.rst

Separately, something is up with our CI provider Travis, where the tests are inexplicably not running for your branch, only the LGTM and Snyk checks are running. I'll look into this in a separate issue.

tests/fixtures/crypto_metadata.yaml Outdated Show resolved Hide resolved
tiingo/api.py Outdated Show resolved Hide resolved
tests/fixtures/crypto_top_of_book.yaml Outdated Show resolved Hide resolved
@hydrosquall
Copy link
Owner

馃殌 This is excellent! Thanks for your contribution @n1rna . This will go out in the 0.12.x release.

@sher85
Copy link

sher85 commented Oct 7, 2020

Hi and thanks for the support with adding crypto endpoints. I could not find an example of the code required to access it on the documentation page, nor when I tried through google. Would you be able to add an example for clarification somewhere?

@hydrosquall
Copy link
Owner

hydrosquall commented Oct 7, 2020

Hi @sher85 - thanks for reaching out!

There are 3 endpoints available (ignore all the lines with assert in them) - you can see a live example of what arguments go into each function in our test code:

https://github.com/n1rna/tiingo-python/blob/fb1295376a1049b8d2d8819a648b82c9f7c0087b/tests/test_crypto.py#L19-L37

Let me know if you have any questions or what additional information will be helpful - I'll make a note (#520 (comment)) to add a section to the README.md dedicated to usage of the Crypto endpoints, and incorporate your feedback (alternately, would be happy to work together with you to write this section if you're interested in contributing documentation).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants