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

[BugFix] Add sort to tiingo equity historical #6261

Closed
wants to merge 1 commit into from

Conversation

hjoaquim
Copy link
Contributor

@hjoaquim hjoaquim commented Mar 27, 2024

  1. Why?:

    • Tiingo requires the sort argument. The endpoint breaks without it.
  2. What? (1-3 sentences or a bullet point list):

    • Added the sort argument.
  3. Impact (1-2 sentences or a bullet point list):

    • 5 (positive UX impact fixing an unusable endpoint)
  4. Testing Done:

    • Run the endpoint for tiingo, before and after the fix.

@github-actions github-actions bot added bug Fix bug platform OpenBB Platform v4 PRs for v4 labels Mar 27, 2024
@@ -44,6 +44,24 @@ class TiingoEquityHistoricalQueryParams(EquityHistoricalQueryParams):
_frequency: Literal["daily", "weekly", "monthly", "annually"] = PrivateAttr(
default=None
)
sort: Literal[
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this a fruitless effort to expose this param? Sorting occurs by date, ALWAYS for this endpoint, and results are always sorted in the same direction, ascending for every provider.

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 got this error without using the sort:
"Error: Sort column name is not a valid column"

So, apparently the sort needs to be explicit in the URL.

Copy link
Contributor

Choose a reason for hiding this comment

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

How come it works for me right now, without this?

Copy link
Contributor

Choose a reason for hiding this comment

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

The URL appears to work fine without a sort column.

https://api.tiingo.com/tiingo/daily/AAPL/prices?startDate=2023-03-27&endDate=2024-03-27&resampleFreq=monthly&token=

Screenshot 2024-03-27 at 10 54 01 AM

@deeleeramone
Copy link
Contributor

I'm not able to recreate the issue you raise on the develop branch:

obb.equity.price.historical("AAPL", provider="tiingo").to_df()

Screenshot 2024-03-27 at 10 43 46 AM

@hjoaquim
Copy link
Contributor Author

Just figured that #6259 fixes this issue - the URL is built differently and then the sort is not mandatory (apparently). Not sure if we should keep it anyway.
@deeleeramone @montezdesousa

@deeleeramone
Copy link
Contributor

Just figured that #6259 fixes this issue - the URL is built differently and then the sort is not mandatory (apparently). Not sure if we should keep it anyway. @deeleeramone @montezdesousa

There is no purpose for a sort field, time series data all needs to be the same from all providers - sorted by date/ascending. If users want to sort by the closing price, they will need to do that post-request..

@hjoaquim hjoaquim closed this Mar 27, 2024
@piiq piiq deleted the bugfix/sort-tiingo-historical branch May 7, 2024 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fix bug platform OpenBB Platform v4 PRs for v4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants