-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
[Feature] New endpoint economy.markets.historical
#6107
Conversation
async def callback(response: ClientResponse, _: Any) -> Union[dict, List[dict]]: | ||
"""Return the response.""" | ||
if response.status != 200: | ||
raise RuntimeError(f"Error in TE request -> {await response.text()}") |
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.
Because we are allowing multiple symbols, the failure of one symbol should communicate via warnings, raising an EmptyDataError
if all fail.
We don't want to throw out the entire request because of one bad apple.
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.
Agreed.
""" | ||
Get historical price data for a symbol within a market category. | ||
|
||
The market categories available are exchange rates, stock market indexes, |
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.
Should this functionality be split up into the existing router functions, like index.price.historical
, crypto.price.historical
, currency.price.historical
?
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.
I agree with darren. This api is wacky, but we can get snapshots without a ticker (f'https://api.tradingeconomics.com/markets/index?c={api_key}' or for the specific symbol:
f'https://api.tradingeconomics.com/markets/historical/aapl:us,gac:com?c={api_key}'
So this should go into snapshot and historical for crypto/index/stock. This also probably warrants a new bond/government/snapshot for f'https://api.tradingeconomics.com/markets/bond?c={api_key}'
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.
That's a very fair point. I don't know tbh, although it covers those assets, it's not limited to them - and TE doesn't seem to offer any options for filtering.
Thoughts @jmaslek @minhhoang1023 ?
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.
The page was outdated when I 1st commented! I agree with that approach @jmaslek. Will restructure accordingly.
So, iiuc:
- TE Historical will serve crypto/index/equity historical.
- TE Snapshot will serve economy/index snapshot. --> should we create a snapshot endpoint under economy for the bonds?
The expectation is that the user can then use (eg) index.historical
to get historical data for any given bond?
Pull Request Template for OpenBB Developers
Why?:
What? :
economy.markets.historical
MarketHistorical
)TEMarketHistoricalFetcher
)Impact (1-2 sentences or a bullet point list):
Testing Done:
Reviewer Notes (optional):
Any other information (optional)