-
-
Notifications
You must be signed in to change notification settings - Fork 384
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: week_of_month returns negative values #718
Conversation
@@ -61,6 +61,9 @@ def test_week_of_month(): | |||
assert pendulum.date(2020, 1, 1).week_of_month == 1 | |||
assert pendulum.date(2020, 1, 7).week_of_month == 2 | |||
assert pendulum.date(2020, 1, 14).week_of_month == 3 | |||
assert pendulum.date(2023, 1, 1).week_of_month == 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct, January 1st 2023 belongs to the last week of December of 2022. Since this is the edge case, it should return 0.
@@ -61,6 +61,9 @@ def test_week_of_month(): | |||
assert pendulum.date(2020, 1, 1).week_of_month == 1 | |||
assert pendulum.date(2020, 1, 7).week_of_month == 2 | |||
assert pendulum.date(2020, 1, 14).week_of_month == 3 | |||
assert pendulum.date(2023, 1, 1).week_of_month == 1 | |||
assert pendulum.date(2023, 1, 2).week_of_month == 2 | |||
assert pendulum.date(2023, 1, 31).week_of_month == 6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct, from an ISO standpoint it's not possible to have a 6th week, it should be 5.
week_days = days_of_week() | ||
first_day_index = week_days.index(first_day_of_month.day_of_week) | ||
days_in_first_week = pendulum.DAYS_PER_WEEK - first_day_index | ||
days_after_first_week = self.day - days_in_first_week | ||
return math.ceil(days_after_first_week / pendulum.DAYS_PER_WEEK) + 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
week_days = days_of_week() | |
first_day_index = week_days.index(first_day_of_month.day_of_week) | |
days_in_first_week = pendulum.DAYS_PER_WEEK - first_day_index | |
days_after_first_week = self.day - days_in_first_week | |
return math.ceil(days_after_first_week / pendulum.DAYS_PER_WEEK) + 1 | |
return int( | |
math.ceil((self.day + first_day_of_month.day_of_week - 1) / DAYS_PER_WEEK) | |
) |
This should give us what we want while taking care of the edge cases that needs fixing.
@@ -61,6 +61,9 @@ def test_week_of_month(): | |||
assert pendulum.date(2020, 1, 1).week_of_month == 1 | |||
assert pendulum.date(2020, 1, 7).week_of_month == 2 | |||
assert pendulum.date(2020, 1, 14).week_of_month == 3 | |||
assert pendulum.date(2023, 1, 1).week_of_month == 1 | |||
assert pendulum.date(2023, 1, 2).week_of_month == 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert pendulum.date(2023, 1, 2).week_of_month == 2 | |
assert pendulum.date(2023, 1, 2).week_of_month == 1 |
Pull Request Check List
Description
Resolves #587
Changes
Using day of month instead of
week_of_year
to calculateweek_of_month
.