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

Wip handle future intervals #371

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

sruffell
Copy link
Contributor

@sruffell sruffell commented Aug 19, 2020

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:

  1. 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.

  2. If there is a future closed interval, the default command will just print There is no active time tracking but entering timew start will print an error. For example:

$ src/timew start proja $(date +%Y-%m-%dT%H:%M:%S -d 'now + 2 hour')
Note: 'proja' is a new tag.
Will start tracking proja
  at 2020-08-19T04:33:49
$ src/timew stop
Recorded proja
  Started 2020-08-19T04:33:49
  Ended                    49
  Total               0:00:00
$ src/timew
There is no active time tracking.
$ src/timew start
You cannot overlap intervals. Correct the start/end time, or specify the :adjust hint.
$ src/timew summary :ids

Wk  Date       Day ID Tags    Start     End    Time   Total
W34 2020-08-19 Wed @1 proja 4:33:49 4:33:49 0:00:00 0:00:00

                                                    0:00:00

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:

@lauft
Copy link
Member

lauft commented Aug 19, 2020

@sruffell Can't we first implement the patch proposed in #366 such that we can finalize 1.4.0 before opening a new field of debate?

@lauft lauft added the discussion Discussion on a topic label Aug 19, 2020
@sruffell
Copy link
Contributor Author

sruffell commented Aug 19, 2020

@sruffell Can't we first implement the patch proposed in #366 such that we can finalize 1.4.0 before opening a new field of debate?

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.

@sl4mmy
Copy link
Contributor

sl4mmy commented Aug 19, 2020

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 timew start when there's a future closed interval isn't intuitive (in my opinion, at least).

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 timew start to be a warning rather than an error, in which case I would argue it also would make sense to add a warning about the future interval when the user runs just timew.

@lauft
Copy link
Member

lauft commented Aug 19, 2020

Personally, I'd like timew start to be a warning rather than an error,...

@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:

$ timew track <2h before> - <1h before> FOO
$ timew start <3h before> BAR
You cannot overlap intervals. Correct the start/end time, or specify the :adjust hint.

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.

Interval [<from> - <to>] "<tag>..." overlaps with
 - @<id> [<from> - <to>] "<tag>..."
 -...
Correct the start/end time, or specify the :adjust hint.

I am not sure what timew should print in case of future intervals, maybe for

  • open intervals:
    $ timew start <future> FOO
    ...
    $ timew
    Tracking FOO
      Starting <future>
    
    (in contrast to "normal" intervals the lines Current and Total are omitted for future intervals)
  • closed intervals:
    $ timew track <future> - <far-future> FOO
    ...
    $ timew
    There is no active time tracking.
    Note: Future intervals ahead!
    

@lauft
Copy link
Member

lauft commented Aug 19, 2020

Since we can start an interval in the future (or continue, modify, or move one), if the user enters timew stop what should happen?

@sruffell In this case we have known case of interval.end < interval.start and the respective error messages should be displayed:

$ timew start <future>
$ timew stop <now>
The end of a date range must be after the start.

Maybe we should refine the error message generally to something like

Interval <tag>..., started at <future> cannot be stopped at <now>.

@sruffell
Copy link
Contributor Author

sruffell commented Aug 25, 2020

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:

$ rm -fr ~/.timewarrior/data
$ src/timew start $(date -d '+5 min' +%Y-%m-%dT%H:%M:%S) FOO BAR BAZ
Note: 'BAR' is a new tag.
Note: 'BAZ' is a new tag.
Note: 'FOO' is a new tag.
Tracking of BAR BAZ FOO
  Scheduled for 2020-08-25T03:33:24
$ src/timew stop FOO
Interval BAR BAZ FOO started at 2020-08-25T03:33:24 cannot be stopped at 2020-08-25T03:28:34.
$ src/timew stop $(date -d '+5 min' +%Y-%m-%dT%H:%M:%S) FOO BAR BAZ
Recorded BAR BAZ FOO
  Started 2020-08-25T03:33:24
  Ended                    49
  Total               0:00:25
$ src/timew
There is no active time tracking but future intervals are present.

@lauft
Copy link
Member

lauft commented Aug 25, 2020

@sruffell I would keep the messages when tracking an interval consistent:

  • "normal" interval
$ timew start FOO
Tracking FOO
  Started 2020-08-25T22:30:11
  Current                  11
  Total               0:00:00
  • future interval
$ timew start FOO <future>
Tracking FOO
  Scheduled <future>

(instead of Tracking of ... Scheduled for...). Same output when just calling timew`.

I would also prefer to separate the feedback from timew from the warning about future intervals:

$ timew
There is no active time tracking.
Note: Future intervals present.

instead of putting everything on one line.

@sruffell
Copy link
Contributor Author

sruffell commented Aug 26, 2020

Just taking a note for myself to also make sure I check the chart command with future intervals (i.e. #370) in addition to the changes lauft requested. Might also be worth changing the error message to handle cases like #402.

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]>
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev-branch discussion Discussion on a topic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants