polygon-io / client-python

The official Python client library for the Polygon REST and WebSocket API.
https://polygon-api-client.readthedocs.io/
MIT License
801 stars 210 forks source link

Inconsistent Type Hinting for Aggregate Date Parameters #680

Closed SuperMaZingCoder closed 3 months ago

SuperMaZingCoder commented 3 months ago

Inconsistent Type Hinting for Aggregate Date Parameters

Context

list_aggs and get_aggs type hint Union[str, int, datetime, date] for their from_ and to parameters:

from_: Union[str, int, datetime, date],
to: Union[str, int, datetime, date],

get_grouped_daily_aggs and get_daily_open_close_agg only type hint str for their date parameters:

date: str,

Problem: get_grouped_daily_aggs and get_daily_open_close_agg are missing the type hints they deserve

datetime.date objects can already be passed to get_grouped_daily_aggs and get_daily_open_close_agg since

f"/v2/aggs/grouped/locale/{locale}/market/{market_type}/{date}"

and

f"/v1/open-close/{ticker}/{date}"

auto-convert them into strings of YYYY-MM-DD format.

This is the same thing that happens in list_aggs and get_aggs, but they actually get type hinting for datetime.date objects. It's a small inconsistency.

Given that it works in all cases, the linting is misleading/inaccurate: image

Argument of type "date" cannot be assigned to parameter "date" of type "str" in function "get_daily_open_close_agg" 
"date" is incompatible with "str"

Solution

I'm aware neither get_grouped_daily_aggs nor get_daily_open_close_agg can support datetime.datetime objects or UNIX Timestamps since their endpoints don't support it, but type hinting should be added for datetime.date objects.

date: Union[str, date],

It's obviously a very, very small problem, but it's annoying me 😭 A fix would be nice so that #type: ignore isn't needed to keep the linter quiet. It's also helpful for people who don't know that string formatting converts datetime.date objects into YYYY-MM-DD format.

Super quick fix. I'll have a PR in a few minutes.

justinpolygon commented 3 months ago

Great suggestion @SuperMaZingCoder! Thanks for the PR too. I'll merge and we're planning a release shortly so that'll roll out with it. Thank you.