[Bug]: StockHistoricalDataClient produces a wrong URL

peters-r commented 11 months ago

Is there an existing issue for this?

[X] I have searched the existing issues

Current Behavior

client.get_stock_bars(StockBarsRequest( symbol_or_symbols=["AAPL"], timeframe=TimeFrame(amount=1,unit=TimeFrame.Day), start=datetime(2023, 9, 7)))

does not succeed. It creates an invalid timeframe by appending an additional digit: https://data.alpaca.markets/v2/stocks/bars?start=2023-09-07T00%3A00%3A00Z&timeframe=11Day&symbols=AAPL

Expected Behavior

client.get_stock_bars(StockBarsRequest( symbol_or_symbols=["AAPL"], timeframe=TimeFrame(amount=1,unit=TimeFrame.Day), start=datetime(2023, 9, 7)))

Shall produce the URL: https://data.alpaca.markets/v2/stocks/bars?start=2023-09-07T00%3A00%3A00Z&timeframe=1Day&symbols=AAPL

SDK Version I encountered this issue in

alpaca_py-0.11.0-py3-none-any.whl

Steps To Reproduce

from alpaca.data.historical import stock
from alpaca.data.requests import StockBarsRequest
from alpaca.data.timeframe import TimeFrame
from datetime import datetime

historical = stock.StockHistoricalDataClient(...)

request_bars = StockBarsRequest(
    symbol_or_symbols=["AAPL"], 
    timeframe=TimeFrame(amount=1,unit=TimeFrame.Day), 
    start=datetime(2023, 9, 7))

historical.get_stock_bars(request_bars)

Filled out the Steps to Reproduce section?

[X] I have entered valid steps to reproduce my issue or have attached a minimally reproducible case in code that shows my issue happening; and understand that without this my issue will be flagged as invalid and closed after 30 days.

Anything else?

No response

sshcli commented 11 months ago

@peters-r try whith the TimeFrameUnit:

from alpaca.data.timeframe import TimeFrame              
from alpaca.data.timeframe import TimeFrameUnit   
StockBarsRequest(symbol_or_symbols=[SYMBOL],timeframe=TimeFrame(1, TimeFrameUnit.Day), start=START, end=END)

alessiocastrica commented 11 months ago

This is expected TimeFrame is not an Enum, but TimeFrameUnit is.

peters-r commented 11 months ago

So it's not a bug, but still I think the API is highly confusing in that regard. Maybe could be worth to at least make sure that TimeFrameUnit and TimeFrame don't have the same 'Day' property or to make it more explicit that one is an enum and the other isn't. Or to update the docs to make it more obvious. I probably wouldn't have filed a bug if I found it in the docs.

It works for me now. Thanks for your quick support!

alessiocastrica commented 11 months ago

@peters-r I agree with you that having the same name can be confusing, but changing the API would be a breaking change that I would prefer to avoid right now. The TimeFrame Day class property was actually imagined as a shortcut, please check out the implementation here. So in your case you can just use timeframe=TimeFrame.Day Hope this helps.

alpacahq / alpaca-py