Skip to content
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 calculation of 'end of previous work week' #73

Merged
merged 2 commits into from
Aug 12, 2023
Merged

Conversation

lauft
Copy link
Member

@lauft lauft commented Aug 12, 2023

The calculation of the end of previous work week (eopww) fails if the current day is a Saturday.
The algorithm (Datetime.cpp:2059) subtracts the current weekday (with Sunday = 0) plus one modulo 7 from the current day of month.

When the current day is a Saturday, this results in a subtraction of 0, i.e. the current day remains unchanged, while it should be the previous Saturday.

Dropping the modulo fixes the issue.

The calculation of the _end of previous work week_ (eopww) fails if the current day is a Saturday.
The algorithm ([Datetime.cpp:2059](https://github.com/GothenburgBitFactory/libshared/blob/ee050a2cc68c46e8748113246b26feb0ad9f34c5/src/Datetime.cpp#L2059-L2063)) subtracts the current weekday (with Sunday = 0) plus one modulo 7 from the current day of month.

When the current day is a Saturday, this results in a subtraction of 0, i.e. the current day remains unchanged, while it should be the previous Saturday.

Dropping the modulo fixes the issue.
@lauft lauft requested review from djmitche and tbabej August 12, 2023 13:19
@lauft lauft self-assigned this Aug 12, 2023
@lauft lauft added the bug Something isn't working label Aug 12, 2023
@@ -52,7 +52,7 @@ void testInit (UnitTest& t, const std::string& value, Datetime& var)
////////////////////////////////////////////////////////////////////////////////
int main (int, char**)
{
UnitTest t (164);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hang on, this just changes the number of expected tests. Let's add a test case for this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The currently failing testcase is here:

t.ok (eopww < soww, "eopww < soww");

in dates.t.cpp. It only fails on a Saturday though...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change her only aligns the number of actually present testcases with the number of planned ones (i.e. the argument of the UnitTest constructor).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a specific test case for "eopww should be before soww on Saturdays" would require the reference date (a.k.a. now) to be injectable into the Datetime parser, which would be preferable, but a bigger endeveaour... 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see -- weird to have date-time tests depend on the actual date and time. Yeah, fuzzing that (or just replacing them with fake time and figuring out the right times to test all the corner cases) sounds like a good, but larger project.

@lauft lauft changed the title Fix calculatioin of 'end of previous work week' Fix calculation of 'end of previous work week' Aug 12, 2023
@lauft lauft merged commit a2475a1 into master Aug 12, 2023
10 checks passed
@lauft lauft deleted the feature/fix-eopww branch August 12, 2023 18:51
smemsh added a commit to smemsh/taskwarrior that referenced this pull request Oct 17, 2024
mainly those visible changes, and miscellaneous others

see GothenburgBitFactory#3623 (weekstart)
see GothenburgBitFactory#3651 (epoch limit defines)
see GothenburgBitFactory/libshared#73 (eopww fix)
djmitche pushed a commit to GothenburgBitFactory/taskwarrior that referenced this pull request Oct 19, 2024
#3654)

* libshared: bump for weekstart, epoch defines, eopww fix

mainly those visible changes, and miscellaneous others

see #3623 (weekstart)
see #3651 (epoch limit defines)
see GothenburgBitFactory/libshared#73 (eopww fix)

* Initialize libshared's weekstart from user's rc.weekstart config

This enables use of newer libshared code that can parse week numbers
according to ISO8601 instead of existing code which is always using
Sunday-based weeks.  To get ISO behavior, set rc.weekstart=monday.
Default is still Sunday / old algorithm, as before, since Sunday is in
the hardcoded default rcfile.

Weekstart does not yet fix week-relative shortcuts, which will still
always use Monday.

See #3623 for further details.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants