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

Update libshared to get new weekstart #3623

Closed
djmitche opened this issue Sep 14, 2024 · 9 comments · Fixed by #3654
Closed

Update libshared to get new weekstart #3623

djmitche opened this issue Sep 14, 2024 · 9 comments · Fixed by #3654
Assignees
Labels
topic:time Issues relating to time
Milestone

Comments

@djmitche
Copy link
Collaborator

From GothenburgBitFactory/libshared#81

The new ISO8601 behavior for Datetime::weekstart == 1 will be a breaking change for users relying on existing week date parsing. This is unavoidable for weekstart == 1. However if the user wants weekstart == 0, the old behavior is still possible if Taskwarrior/Timewarrior were modified to set Datetime::weekstart before libshared calls (which they currently do not). Taskwarrior could propagate its rc.weekstart value (used only for task calendar so far) and propagate this value to libshared, for example.

@djmitche djmitche self-assigned this Sep 14, 2024
@djmitche djmitche moved this from Backlog to Ready in Taskwarrior Development Sep 14, 2024
@djmitche djmitche added this to the v3.2.0 milestone Sep 14, 2024
@smemsh
Copy link
Contributor

smemsh commented Sep 16, 2024

@djmitche Thanks for picking this up, I was planning to make an issue to bump libshared in Taskwarrior, now that the weekstart change was merged.

Note that the libshared change only affects week number parsing, whereas sonw/sopw/etc should still use Monday regardless of Datetime::weekstart (as they did before). I did not look into tackling that but it would be necessary to make those respect Datetime::weekstart as well in order for support to be truly complete.

It looks like all these functions would need changes for that:

  • initializeSopw
  • initializeSow
  • initializeSonw
  • initializeEopw
  • initializeEow
  • initializeEonw
  • initializeSopww
  • initializeSonww
  • initializeSoww
  • initializeEopww
  • initializeEonww
  • initializeEoww

When were you targeting 3.2 ? I might be able to look into doing this, depending on your timeline.

@djmitche
Copy link
Collaborator Author

Ah, that sounds like more work than I anticipated! If you can work on it, that would be great. I think it's fine for this to be "done when it's done" so don't rush to get it into 3.2.0.

@djmitche djmitche assigned smemsh and unassigned djmitche Sep 16, 2024
@smemsh
Copy link
Contributor

smemsh commented Sep 16, 2024

It's not strictly necessary to get those additional changes in. Merely updating the libshared peg and propagating rc.weekstart to the library would still be a better situation than today, it just would leave the sonw/sopw/etc aliases hard-configured to Monday weeks, but they already are (so at least parsing weeks would be made better, currently they are hard-configured to Sunday weeks).

Let me look at how hard I think it would be to fix the aliases too and I'll get back to you on whether an intermediate release doing the more limited change is warranted in the meantime. Give me a little bit, thanks.

@djmitche
Copy link
Collaborator Author

Sounds good, take your time :)

@elig0n
Copy link

elig0n commented Sep 24, 2024

Before libshared there was this code which took weekstart into consideration:
903b5b3

Maybe it can assist any of you

@djmitche
Copy link
Collaborator Author

It looks like GothenburgBitFactory/libshared#83 will need a libshared update as well. Do you want to do those updates separately or at the same time?

@smemsh
Copy link
Contributor

smemsh commented Oct 15, 2024

Let's do the lesser for now, I'm changing continents at the moment. I'll look at the rest later.

So to bump libshared and have least-surprise, I suggest we pass in the user's rc.weekstart as the value to initialize libshared's Datetime::weekstart. Previous code always behaved as the new code does when weekstart=0, but now it would use the [previously not consulted] initialization at the top of Datetime.cpp (which is weekstart=1), so we should instead prime it from user config.

I think I have that working, but when I ran the TW tests, it revealed a bug in the new libshared code. Could you merge GothenburgBitFactory/libshared#84 first (and also the GothenburgBitFactory/libshared#83 you mentioned, if you're planning on it)? And then I have a patch that passes down rc.weekstart to Datetime::weekstart as described, it seems to work. I'll include the final libshared peg update in my PR.

@djmitche
Copy link
Collaborator Author

I've merged both of those PRs.

smemsh added a commit to smemsh/taskwarrior that referenced this issue 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)
smemsh added a commit to smemsh/taskwarrior that referenced this issue Oct 17, 2024
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

smemsh commented Oct 17, 2024

Thanks @djmitche. Take a look at #3654, which should bring in the libshared master branch tip, and close this one. Then #3651 can proceed.

djmitche pushed a commit that referenced this issue 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.
@github-project-automation github-project-automation bot moved this from Ready to Done in Taskwarrior Development Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic:time Issues relating to time
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants