Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15 +/- ##
==========================================
+ Coverage 93.95% 94.11% +0.15%
==========================================
Files 12 12
Lines 331 340 +9
==========================================
+ Hits 311 320 +9
Misses 20 20 ☔ View full report in Codecov by Sentry. |
lpicci96
left a comment
lpicci96
left a comment
There was a problem hiding this comment.
Thanks @mharoruiz
2 changes I think we should make
| latest_date_obj = datetime(latest_date[0], latest_date[1], 1) | ||
| if date_obj > latest_date_obj: | ||
| logger.info( | ||
| f"SDR data unavailable for date: {format_date(date[1],date[0])}. Will fetch latest available" |
There was a problem hiding this comment.
I'm not sure we should return data as this might lead to silent errors. It might be better to raise a value error noting data for the date is unavailable, and specify the latest available date
| A dataframe with the SDR allocations and holdings data | ||
| """ | ||
|
|
||
| latest_date = get_latest_allocations_holdings_date() |
There was a problem hiding this comment.
A minor issue. This function logs a message saying it is retrieving the latest date. This doesn't make sense when a user requests a specific date.
There was a problem hiding this comment.
I added a boolean option to suppress the logging info in get_latest_allocations_holdings_date(). Not sure if this is the best workaround though.
There was a problem hiding this comment.
I don't think this is the best approach, because it isn't very useful argument for the user to be exposed to. I suggest within the fetch_allocations_holdings function you temporarily disable logging. The functionality should be:
- if the user does not set a date or requests the latest date, it should log the message "fetching latest date"
- if the user specifies the date, it should not log that message, only the "fetching data" message. To do this disable the logger before the fetch_latest_date function is called, then reset the logger after it is called
|
|
||
| @lru_cache | ||
| def get_latest_allocations_holdings_date() -> tuple[int, int]: | ||
| def get_latest_allocations_holdings_date(log_info=True) -> tuple[int, int]: |
There was a problem hiding this comment.
Just noticed, this should be called "fetch..." to be consistent with the other user facing functions
| # Temporarily disable logging while calling fetch_latest_allocations_holdings_date() | ||
| logger.setLevel(logging.WARNING) | ||
| latest_date = fetch_latest_allocations_holdings_date() | ||
| logger.setLevel(logging.INFO) |
There was a problem hiding this comment.
This approach works. The only consideration is that you are hard coding the level back to INFO which is not the safest approach. You should rather catch what the original level was - something like original_level = logger.level and after completion of the task, set it back to original_level
3 participants
Added a date comparison to
fetch_allocations_holdings()when no date is provided to make sure the output reflects the latest date available if the provided one is further in the future.