-
Notifications
You must be signed in to change notification settings - Fork 95
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
Make week number parsing ISO8601 compliant #633
Merged
lauft
merged 3 commits into
GothenburgBitFactory:develop
from
smemsh:libshared-sync-isoweeks
Sep 13, 2024
Merged
Make week number parsing ISO8601 compliant #633
lauft
merged 3 commits into
GothenburgBitFactory:develop
from
smemsh:libshared-sync-isoweeks
Sep 13, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This corrects for some drift in our shadowed copy of libshared's Datetime::validate (in our own DatetimeParser class with function of same name), from libshared commit 7dffb13c Signed-off-by: Scott Mcdermott <[email protected]>
…== 1 This is a duplicate of the changes from commit with same subject line in GothenburgBitFactory/libshared#81 and also duplicates the commit message below. Timewarrior has copies of many functions from libshared's Datetime class in its own DatetimeParser class, and until such time as these classes are integrated, we have to maintain copies of these functions here, and the changes need to be duplicated. See discussion in aforementioned PR. The one difference with the patch over there is, this one is using the public Datetime::dayOfWeek() and Datetime::daysInYear() methods from libshared, rather than its own implementation. The copy in Timewarrior already was doing this before, but it's worth noting it's the only difference with the corresponding patch in libshared PR 81, and only amounts to a change in the namespace qualifier. Copied commit message from libshared follows. * * * This patch makes the parsing of week numbers in dates ISO-8601 compliant in the case that Datetime::weekstart == 1, while the existing behavior remains available if Datetime::weekstart == 0. The previous code parsed week numbers (given as "yyyy-Www") into dates starting on Sunday. Although the code had a "Datetime::weekstart" variable, and this value was initialized to 1 (which is Monday) in libshared, nonetheless week specifications would be parsed into calendar days starting on Sunday. Furthermore, week days in such given weeks ('d' in "yyyy-Www-d") used 0-6 for Sunday-Monday, while ISO8601 specifies 1-7 Monday-Sunday. Therefore, yyyy-Www-0 was treated as valid (and should not be), while yyyy-Www-7 was invalid (and should be valid). Note that neither Taskwarrior nor Timewarrior ever set the value of Datetime::weekstart. Taskwarrior has an rc.weekstart, but this is only used for "task calendar" output, not for parsing dates. The patch does the following: - Initialize "Datetime" instances with a weekday value from Datetime::weekstart. This helps the case when weekday is not supplied, it won't default to zero and fail validation (when weekstart is '1'). Note that mktime() is usually used in the code to convert populated "struct tm" broken-down times into epoch-times, and this operation does not use tm.tm_wday for input, only as an output field, recomputed as a normalized value, so it appears to be safe to initialize it with a 1 (which we might wonder about since .tm_wday is supposed to be 0-6 Sunday based). - Use the already-existing Datetime::parse_weekday() to parse the 'ww' in "yyyy-Www" input dates (the function was not used by any caller previously; guessing it may have been intended for eventual use in order to respect weekstart(?)) - Teach parse_weekday() about weekstart. Previously this assumed 1-7, which is the reason I'm guessing this was intended to be used for ISO weeks eventually. Now it can also use 0-6 if weekstart 0. - Teach Datetime::validate to respect Datetime::weekstart also. Previously only 0-6 Sunday-Monday was considered valid. - Default the weekday to Datetime::weekstart if not supplied, ie for "yyyy-Www-X" if the "-X" is not supplied, as recognized when Datetime::standaloneDateEnabled = true, which is the case for (1) tests, (2) timewarrior, but NOT for taskwarrior at this time (both the standalone 'calc' and 'task calc' (ie Context.cpp) set this to false). - Implement the complete ISO week calculation algorithm in Datetime::resolve() if weekstart is '1', and keeps the existing algorithm if weekstart is '0'. This will allow Taskwarrior and Timewarrior to offer the option of the old behavior for those who want to use Sunday-based weeks and ignore ISO week calculations (such as 2023-01-01 being in ISO week 2022-W52). Signed-off-by: Scott Mcdermott <[email protected]>
Waiting for GothenburgBitFactory/libshared#81 to get merged first. I will then update the libshared peg, add to this PR, and undraft so we can merge this one. |
Brings in new dayOfWeek() code from GothenburgBitFactory/libshared#81 Signed-off-by: Scott Mcdermott <[email protected]>
Thanks @lauft! The libshared peg has been advanced, this one is ready. |
Thanks ❤️ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes week number parsing ISO8601-compliant. Previously, week numbers parsed as Sunday-based when given as
2024-W34
, and did not respect the ISO8601 algorithm for which year they were in. This was inconsistent with:week
,:lastweek
, etc, which use Monday-based weeks. This PR equalizes that inconsistency, and switches to the ISO standard algorithm for week numbers.As a side effect, note that day number parsing (ie
2024-W34-1
) also changes from 0-6 Sunday to 1-7 Monday, again following ISO8601. This only has a consequence for Sunday itself, which changes from day 0 to day 7.NOTE: These changes strictly duplicate ones from GothenburgBitFactory/libshared#81. Timewarrior contains duplicate implementations of several methods from libshared's
Datetime
class in its ownDatetimeParser
class. All methods we have changed are identical in both places. Eventually, the two classes are intended be merged back together.The only difference with the libshared version of the patch is that we use libshared's
Datetime::dayOfWeek()
andDatetime::daysInYear()
because they aren't among the copied methods in ourDatetimeParser
class.Rationale and details, including references and links to standards, can be seen in the discussion and commits in libshared.
No tests were added to Timewarrior, as all my changes are already fully covered in new libshared tests, and all the existing tests here pass with no changes.
Fixes #595.