-
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
Fundamentals metadata #596
base: master
Are you sure you want to change the base?
Conversation
This pull request introduces 1 alert when merging e5fb0ea into 4cfca69 - view on LGTM.com new alerts:
|
Hey @RJTK , Thanks so much for this contribution. I think you're right that 2.6 and 2.7 are causing issues and it's not worth the maintenance burden given that 2 was sunset at the end of last year. I also use I think this will also let us simplify the parts of the code that check for whether users are running python 2 or 3. Let me open up an issue to sunset Python 2, with some of the checklist items that are needed for us to switch - I'd be happy to review any PRs you have time for to help with that effort, otherwise I'll try to look into this more on the weekend. |
Opened #601 with a shortlist of things needed to phase out python 2. |
Cool thanks -- I will look at #601 and see if I can address any of the points. I am not well versed in these integration tools, but would probably be well advised to learn. |
@RJTK I did some cleanup this weekend, can you try rebasing your branch against the latest
My concern is that the two endpoints may not have identical signatures, and we could end up with parameters in the function that are only meant to be used by one or the other. For the moment, I think it's OK for us to keep the pattern of roughly 1 endpoint per function for these metadata calls to keep things interpretable, even though the get_ticker_price is not really following this rule (since it uses a different endpoint for intraday data).
Brilliant, thank you!
Ah, thanks for looking into this optimization. What are your thoughts on what a user should do if they want to invalidate the cached results? |
This makes sense to me.
This didn't occur to me -- I added an option to force it to refresh the cache and updated some comments to make it clear that the results were cached. We could also just get rid of this caching -- my assumption was that it wouldn't be asking the website for the csv file each time the method was called, and that it was basically just a "getter" on a class attribute -- but I realize now that the opposite assumption may also be natural. I think some tests are failing on the CI because there's no API key? How do I update the fixtures without committing my API key to git? Just set the key to 00000000... ? |
Hi @RJTK , we use a project called Once you have your fixture, you can use the API key cleaning tool introduced in to replace your API key from the response with the The discoverability of our docs/test-writing structure can be improved, but for now everything you need should be in the tips section: https://github.com/hydrosquall/tiingo-python/blob/master/CONTRIBUTING.rst#tips .
We've erred on the side of not caching so as to avoid returning stale results. If we do have caching as an option, I'd encourage us towards having it be disabled by default so that anyone who currently expects to get fresh results on every call won't have to edit their programs to use the new parameter (this ensures backwards compatibility). Let me know if I can help with anything else. Thanks for following up on this! |
Codecov Report
@@ Coverage Diff @@
## master #596 +/- ##
===========================================
- Coverage 93.72% 82.73% -11.00%
===========================================
Files 5 5
Lines 255 249 -6
===========================================
- Hits 239 206 -33
- Misses 16 43 +27
Continue to review full report at Codecov.
|
I removed caching for the ticker symbols and commit the test fixtures. |
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.
Hi @RJTK , this is looking good, thanks for looking into VCR.py , the CI is nice and 🟢 now. Now that they're passing, I was able to take a closer look at the actual implementation. Let me know if you have any questions.
As an aside, the good work you did with consolidating the response formatters will make it easier for us to centralize having friendlier error messages when the response turns out to be something besides the intended JSON , so thank you for having the foresight to improve that! #159
Hi @RJTK , just wanted to check on the status of your PR / if you had any questions. |
Hi yes, sorry -- I actually just completely forgot about this since I had what I needed working and moved onto another aspect of the project. Thanks a lot for the feedback -- I will update the PR by tomorrow. |
No rush, and glad to hear things worked out in your project :)! Thanks for
taking the time to contribute upstream rather than just updating your local
copy, it’s much appreciated (especially since it probably ended up adding
more steps than originally anticipated).
…On Sat, Apr 10, 2021 at 5:44 PM RJTK ***@***.***> wrote:
Hi yes, sorry -- I actually just completely forgot about this since I had
what I needed working and moved onto another aspect of the project. Thanks
a lot for the feedback -- I will update the PR by tomorrow.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#596 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACE2MM4EAQDZVCTYI42OGCTTIDBCNANCNFSM4YGYO3KQ>
.
|
What does it mean to run a workflow? From the docs it says that "Forks of public repositories can submit pull requests that propose changes to a repository's GitHub Actions workflows" -- have I modified a file that affects something here? I changed the test tickers to PG and CAT -- these are part of the DOW30 and seem to be more freely available, so users that don't have or don't have yet premium keys will hopefully have fewer problems. |
I think there was a recent change to how github actions work due to issues with cryptocurrency mining abuse. I've clicked 'approve', so you shouldn't have any issues with this. https://github.blog/2021-04-22-github-actions-update-helping-maintainers-combat-bad-actors/ For future reference, running a workflow just means running the commands in https://github.com/hydrosquall/tiingo-python/blob/master/.github/workflows/python-package.yml, which happens based on the events specified in these lines: tiingo-python/.github/workflows/python-package.yml Lines 6 to 10 in ff845a2
Ah, thank you! FWIW generally users shouldn't need to have a tiingo api account (or even internet connection) to be able to run the unit tests, this is the purpose of recording the API responses as fixtures. With that said, I don't have particular attachment to GOOG or APPL, so switching to these tickers is fine with me. From the looks of it, there might be some additional tests to update https://github.com/hydrosquall/tiingo-python/pull/596/checks?check_run_id=2487772634 The intraday pricing cassette that was re-recorded also needs to have its ticker updated from GOOG since it's used by the tiingo-python/tests/test_tiingo_pandas.py Lines 110 to 116 in ff845a2
|
Hi @RJTK - just doing some project maintenance and wanted to see if you might have the chance to look into the current test issues . My guess is that since you changed the test tickers, the old cassette fixtures will need to be regenerated. |
Hello, I created a method to query the
fundamentals/metadata
endpoint. I'm not well versed in using some of the development tools, so some stuff is still missing: I have not yet been able to gettox
andpyenv
to run with python versions below 3.5. The formatting check also fails.Can I suggest that support for python 2.6 and 2.7 is dropped? It seems a lot of extra hassle to test against these ancient versions.
Perhaps this method should also just replace or be combined with
get_ticker_metadata
? However, they are two separate endpoints.Summary of Changes
get_fundamentals_meta
._format_response
_ticker_list
so that all thelist_tickers
functions don't collect the zip file multiple times.Checklist
HISTORY.rst
with an entry summarizing your change