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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add columns as parameter to get_dataframe #1057

Merged
merged 13 commits into from
Nov 17, 2024

Conversation

BloodyHell
Copy link
Contributor

@BloodyHell BloodyHell commented Oct 22, 2024

Allow the user to specify which columns to retrieve

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

Summary of Changes

  • What is the motivation / context for this change?
    -- I want to retrieve historical information including volume, but the official API specifies that to retrieve the volume column, it needs to be specified explicitly

  • Is this a bugfix or a feature?
    -- Feature

  • What does this change / fix? (Link to Github Issue page if applicable)
    -- It adds the option for users to include the 'columns' query parameter

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

Allow the user to specify which columns to retrieve
@BloodyHell
Copy link
Contributor Author

@hydrosquall

@hydrosquall
Copy link
Owner

Hi @BloodyHell , thanks for making this contribution. I see from the API docs that this is indeed a new request parameter. I have a few questions for you.

  1. Do you think we should have this param added to the get_ticker_price method as well?
  2. In order to test this change, we should make sure we have a new unit test, and confirm with a live example that the response in fact changes when we pass at least 2 columns. Do you know what columns are available besides volume that we can use to test this? We can list some of the choices in the docstring.

@BloodyHell
Copy link
Contributor Author

Fair points @hydrosquall.

  1. I've added the variable to get_ticker_price as well. Some of that logic could possibly be reused, but that's a change for another moment
  2. I've added two tests, one which requests all columns (all defaults ones including volume) and one only requesting volume. By default you always get date. Also tried to add nice docstring.

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.

The change looks good, thank you for making this update + providing tests for both cases!

I've left some optional feedback if you'd like to make any last changes, but otherwise I can take care of that when we cut a new release.

If you'd like we can add a "volume" usage example to the README.rst at the repository root so people can make use of your parameter, but otherwise people will find out about this param from checking the docstring in their IDE.

HISTORY.rst Outdated Show resolved Hide resolved
tests/test_tiingo.py Show resolved Hide resolved
tests/test_tiingo_pandas.py Show resolved Hide resolved
@BloodyHell
Copy link
Contributor Author

BloodyHell commented Oct 28, 2024

@hydrosquall I've processed your comments and also expanded the README a little bit. Feel free to make changed to it or propose changes as you see fit.

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.

Thanks for the testing + documentation improvements 👍

README.rst Outdated

You can specify any of the end of day frequencies (daily, weekly, monthly, and annually) or any intraday frequency for both the ``get_ticker_price`` and ``get_dataframe``
methods. Weekly frequencies resample to the end of day on Friday, monthly frequencies resample to the last day of the month, and annually frequencies resample to the end of
day on 12-31 of each year. The intraday frequencies are specified using an integer followed by "Min" or "Hour", for example "30Min" or "1Hour".

It's also possible to specify which columns you're interested in, for example: "open", "close", "low", "high" and "volume" (see https://www.tiingo.com/documentation/iex for future columns).
Copy link
Owner

Choose a reason for hiding this comment

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

I think the documentation endpoint for this link is https://www.tiingo.com/documentation/end-of-day rather than iex, but I can fix this after I merge your PR.

tests/test_tiingo_pandas.py 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.

It looks like something might be off with the new unit tests - can you look into what might have happened?

https://github.com/hydrosquall/tiingo-python/actions/runs/11645778111/job/32429181309?pr=1057

@BloodyHell
Copy link
Contributor Author

It looks like something might be off with the new unit tests - can you look into what might have happened?

https://github.com/hydrosquall/tiingo-python/actions/runs/11645778111/job/32429181309?pr=1057

Got it. Requesting it as pandas always converts date to the index of the dataframe, thus it doesn't show up in the columns. Expanded the test_tiingo.py case for multiple columns in json format.

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.

tests/test_tiingo_pandas.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.

We are good to go! Thank you very much for your contribution, as well as for improving documentation and testing along the way.

@hydrosquall hydrosquall merged commit a066830 into hydrosquall:master Nov 17, 2024
9 checks passed
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.

2 participants