-
Notifications
You must be signed in to change notification settings - Fork 96
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
Wip handle future intervals #371
base: dev
Are you sure you want to change the base?
Wip handle future intervals #371
Conversation
If you were intending to release 1.4.0 here soon, then I don't see why not. Just keep in mind that the same change will need to be made to CmdMove and CmdModify as it is possible to setup a future open interval with those commands in the same fashion as CmdContinue. |
186c3d2
to
a3b79b1
Compare
Just my two cents as a user who would appreciate this functionality: I understand your desire to avoid special-casing future intervals, however the behavior of of My use case would simply be when I know I have a meeting starting in a few minutes I might go ahead and start a future open interval now so I don't forget to do it once the meeting actually starts. So in practice I doubt I would actually hit this condition, but it could happen. Personally, I'd like |
@sl4mmy A warning would mean the command was able complete successfully, which in case of (unresolved) overlaps is not allowed to happen. So an error it is. Even without future intervals one can construct something quite similar with the current version of Timewarrior:
Add 3h to every timecode and you have the situation for future intervals. I would like to treat those cases as what they are, overlaps, and therefore issue the same error message regardless of time. The important part here in my opinion is to provide a meaningful error message to avoid confusion, e.g.
I am not sure what
|
@sruffell In this case we have known case of
Maybe we should refine the error message generally to something like
|
a3b79b1
to
e05a886
Compare
Still WIP since there are tests for neither modify nor move, but I rebased this branch on current master and tweaked some of the messages to more closely match @lauft's suggestions:
|
@sruffell I would keep the messages when tracking an interval consistent:
(instead of I would also prefer to separate the feedback from
instead of putting everything on one line. |
Signed-off-by: Shaun Ruffell <[email protected]>
If there is a future open interval scheduled, we do not want synthetic intervals to be flattened into "real" intervals until after the start time of the future interval. Signed-off-by: Shaun Ruffell <[email protected]>
Signed-off-by: Shaun Ruffell <[email protected]>
Now open intervals in the future will just be shown with a duration of zero in the report. It also eliminates how to handle changing the tag set with the future intervals for now. Signed-off-by: Shaun Ruffell <[email protected]>
Signed-off-by: Shaun Ruffell <[email protected]>
Signed-off-by: Shaun Ruffell <[email protected]>
Signed-off-by: Shaun Ruffell <[email protected]>
e05a886
to
07802a5
Compare
This is a different approach to fixing #364 and would replace PR #366.
It is a draft pull request because there are still some additional tests to verify the move and modify command will handle open intervals.
There are two potential areas of debate:
Since we can start an interval in the future (or continue, modify, or move one), if the user enters
timew stop
what should happen? The way it is implemented on this branch is that it stops at the same time creating a closed empty interval in the future. Some alternatives were to delete the interval, or print an error that indicates future intervals must be deleted. But I was trying to minimize any special handling for future intervals.If there is a future closed interval, the default command will just print
There is no active time tracking
but enteringtimew start
will print an error. For example:I wonder if the default command should warn the user that there are future closed intervals or not?
This PR is wip since it depends on: