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

Pass rc.weekstart to libshared for ISO8601 weeknum parsing if "monday" #3654

Merged

Conversation

smemsh
Copy link
Contributor

@smemsh smemsh commented Oct 17, 2024

Libshared master branch has been updated to have some consideration of Datetime::weekstart: date parsing (in particular week/day numbers) will behave differently depending on whether that value is 0 or 1 (see GothenburgBitFactory/libshared#81).

Previously, that code always behaved as the new code does when weekstart=0 when parsing dates (weekstart was used only for task calendar). The new code will use ISO8601 date parsing if that value is 1, which it is initialized to in the top of the file. Since this will cause a change in behavior, we can least surprise users by initializing it from user config, which is done in Context::StaticInitialization().

The hardcoded default rcfile sets weekstart to Sunday (0, the existing behavior) so only users that set rc.weekstart=monday explicitly will see new ISO8601 date parsing behavior, everyone else should see the same behavior as before.

Note: the week shortcuts (sow, sonw, etc) will still always use Monday weeks, regardless of weekstart, until the additional work mentioned in #3623 is done. That will be left for a part2 some time later. We can track that work in #3629.

Old code did not distinguish weekstart, and always used Sunday week parsing:

 $ task3.old rc.weekstart=sunday calc 2013-W49
2013-12-01T00:00:00

 $ task3.old rc.weekstart=monday calc 2013-W49
2013-12-01T00:00:00

The date 2013-12-01 is actually in week 48 for both Sunday and Monday-based weeks, and week 49 only begins on the 2nd, which is seen correctly in ISO-8601 weeks:

 $ date -d 2013-12-01 +s:%U,m:%V
s:48,m:48

 $ date -d 2013-12-02 +s:%U,m:%V
s:48,m:49

This is reflected in the new code when Monday is selected as the weekstart:

 $ src/task rc.weekstart=sunday calc 2013-W49
2013-12-01T00:00:00

 $ src/task rc.weekstart=monday calc 2013-W49
2013-12-02T00:00:00

 $ env - ncal -w -M 12 2013
    December 2013
Mo     2  9 16 23 30
Tu     3 10 17 24 31
We     4 11 18 25
Th     5 12 19 26
Fr     6 13 20 27
Sa     7 14 21 28
Su  1  8 15 22 29
   48 49 50 51 52  1

The old code is left intact when Sunday-based weeks are used. It seems to be Taskwarrior's own definition of weeks, since it differs from strftime() expansion %U, as noted above. Nonetheless, the Sunday behavior was left unchanged from previous Taskwarrior versions, and that remains the default. For correctness, it is recommended for users to configure Monday-based weeks, which would now be determined using an ISO-8601 standard algorithm.

Please apply, thanks.

  1. Fixes Update libshared to get new weekstart #3623 (brings in new weekstart code from libshared)
  2. Fixes ISO week starts on wrong day in task calc output #2922 (considers weekstart when parsing week numbers)
  3. Unblocks Limit the allowed epoch timestamps #3651 (updates libshared peg to current master)
  4. Enables 2.6.X: eow does not respect weekstart #3629 (part1 done for parsing; more work needed)

mainly those visible changes, and miscellaneous others

see GothenburgBitFactory#3623 (weekstart)
see GothenburgBitFactory#3651 (epoch limit defines)
see GothenburgBitFactory/libshared#73 (eopww fix)
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 GothenburgBitFactory#3623 for further details.
@smemsh
Copy link
Contributor Author

smemsh commented Oct 17, 2024

Eventually we should add some tests for weekstart=1. The existing tests probably do not set weekstart except for the calendar, which was the only place weekstart was used before (internally in CmdCalendar only). So, weekstart=0 should be tested enough, but perhaps not weekstart=1.

However, note that I did duplicate all the tests under both weekstarts in libshared itself, and those all pass. So I will probably not add weekstart=1 tests into Taskwarrior until I look at part2, ie #3629.

Copy link
Collaborator

@djmitche djmitche left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed work on this!

@djmitche djmitche merged commit 3e20ad6 into GothenburgBitFactory:develop Oct 19, 2024
14 checks passed
@smemsh smemsh deleted the libshared-weekstart-part1 branch October 23, 2024 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update libshared to get new weekstart ISO week starts on wrong day in task calc output
2 participants