Conversation
There was a problem hiding this comment.
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 suggestions.
Comments skipped due to low confidence (1)
featuremanagement/_time_window_filter/_recurrence_validator.py:113
- The error message should specify the exact parameters for clarity, e.g., 'The Recurrence.Range.EndDate should be after the Start date'.
raise ValueError("The Recurrence.Range.EndDate should be after the Start")
mrm9084
requested review from
albertofori,
avanigupta,
rossgrambo and
zhiyuanliang-ms
|
|
||
| def _sort_days_of_week(days_of_week: List[int], first_day_of_week: int) -> List[int]: | ||
| sorted_days = sorted(days_of_week) | ||
| return sorted_days[sorted_days.index(first_day_of_week) :] + sorted_days[: sorted_days.index(first_day_of_week)] |
There was a problem hiding this comment.
What will happen if first_day_of_week is not in the list?
There was a problem hiding this comment.
It is guaranteed to be. In the RecurrencePattern init we validate that it is, and if it isn't we default to the index value of 0, which is Sunday.
There was a problem hiding this comment.
In python Sunday index is 6, isn't it?
The first_day_of_week is Sunday (6) by default.
Let's say days_of_week is [0, 1] (Mon and Tue)
Won't _sort_days_of_week break as Sunday (6) is not in the list?
There was a problem hiding this comment.
For my original reply, when I say 0 is the default, i.e. Sunday, That was prior to a commit, which I updated the list to start with Monday, which it does in .Net.
There was a problem hiding this comment.
Also, if you look at the new sorted function, it fixes this by finding the closest day of the week to the first day of the week if the first day of the week isn't in the list.
| for day in days_of_week: | ||
| self.days_of_week.append(self.days.index(day)) | ||
| first_day_of_week = pattern_data.get("FirstDayOfWeek", "Sunday") | ||
| self.first_day_of_week = self.days.index(first_day_of_week) if first_day_of_week in self.days else 0 |
There was a problem hiding this comment.
from email.utils import parsedate_to_datetime
s = "Sun, 05 Jan 2025 20:00:00 GMT"
t = parsedate_to_datetime(s)
print(t.weekday())This produced 6 on my laptop, I am using python 3.11.9.
first day of week should be Sunday by default. Sunday index is 6 and Monday is 0.
There was a problem hiding this comment.
I've thought about this and I don't want to mess with the days of the week in python. So, what I think should be fine is that I update. RecurrencePattern to start on Monday which is correct in python, but still have the default first day of the week to be "Sunday" and as the input and default are both strings and we get the index from the list we should be fine. Thoughts?
|
Can we add more testcases like this PR did? |
I looked into it a bit, seems like a pylon bug in 3.8. I've added an empty init to the test folder which seems to have fixed it. Not sure why it wasn't an issue till now. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a recurring time window filter to the Python Feature Management library. Key changes include implementing recurrence models, validators, and evaluators, as well as updating default filter logic and adding comprehensive tests.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/time_window_filter/test_time_window_filter_models.py | Added tests for validating recurrence pattern and range models. |
| tests/time_window_filter/test_recurrence_validator.py | Introduced tests for recurrence settings validation. |
| tests/time_window_filter/test_recurrence_evaluator.py | Provided tests for recurrence evaluator behavior. |
| featuremanagement/_time_window_filter/_recurrence_validator.py | Implemented validation logic for recurrence settings. |
| featuremanagement/_time_window_filter/_recurrence_evaluator.py | Implemented evaluator logic for checking matching time window occurrences. |
| featuremanagement/_time_window_filter/_models.py | Defined recurrence model classes with pattern and range support. |
| featuremanagement/_time_window_filter/init.py | Exposed the recurrence filter functionality. |
| featuremanagement/_defaultfilters.py | Updated default filter to incorporate recurrence data and evaluation. |
| except TypeError as e: | ||
| # Python 3.9 and earlier throw TypeError if the string is not in RFC 2822 format. | ||
| raise ValueError(f"Invalid value for EndDate: {end_date_str}") from e | ||
| self.num_of_occurrences = range_data.get("NumberOfOccurrences", math.pow(2, 63) - 1) |
There was a problem hiding this comment.
Using math.pow(2, 63) returns a float, which may lead to type inconsistencies. Consider using the integer expression (2 ** 63 - 1) to ensure the default occurrence count is an integer.
| if start > day_with_min_offset: | ||
| num_of_occurrences = 0 | ||
| day_with_min_offset = start | ||
| if now < day_with_min_offset: |
There was a problem hiding this comment.
We don't guard that start <= now in this function. But I think it is ok
Co-authored-by: Zhiyuan Liang <141655842+zhiyuanliang-ms@users.noreply.github.com>
…oft/FeatureManagement-Python into RecurringTimeWindowFilter
3 participants
Description
Add the Recurring time window filter to python Feature Management.