fix get_next_market_open/close #33
Conversation
cctdaniel
commented
Jun 21, 2023
cctdaniel
commented
- fix get_next_market_open/close logic
- bump
pythclient/calendar.py
Outdated
| return next_market_close.astimezone(UTC_TZ).strftime("%Y-%m-%dT%H:%M:%S") + "Z" | ||
|
|
||
| print(get_next_market_close("equity", datetime.datetime.now())) |
There was a problem hiding this comment.
can you please add some unit tests for this
|
|
||
|
|
||
| @pytest.fixture | ||
| def equity_open_weekday_datetime(): |
There was a problem hiding this comment.
I think you can declare these as constants instead of using pytest.fixture, and that will save you some code.
| ): | ||
| # equity within market hours | ||
| assert ( | ||
| get_next_market_open("equity", equity_open_weekday_datetime) |
There was a problem hiding this comment.
kind of seems like get_next_market_open should return a datetime?
There was a problem hiding this comment.
or at a minimum extract a test method that does the formatting & comparison so you don't have to repeat this code.
|
|
||
| # equity out of market hours | ||
| assert ( | ||
| get_next_market_open("equity", equity_close_weekday_datetime) |
There was a problem hiding this comment.
it's hard for me to understand how good these tests are because I have to keep scrolling back and forth between the variable definitions and the results. I suggest naming the input variables something like monday_june_6_23 instead, so that the reader can check that the output makes sense without scrolling up.
| return True | ||
| return False | ||
|
|
||
| if asset_type in ["fx", "metal"]: | ||
| if date in FX_METAL_HOLIDAYS: | ||
| return False | ||
| # On Friday the market is closed after 5pm | ||
| if day == 4 and time > FX_METAL_OPEN_CLOSE_TIME: | ||
| if day == 4 and time >= FX_METAL_OPEN_CLOSE_TIME: |
There was a problem hiding this comment.
so what's the semantics if you query exactly at the close time? is the market closed then or open? (I ask because it's worth documenting that)
