timewarrior icon indicating copy to clipboard operation
timewarrior copied to clipboard

Add command `annotation`

Open lauft opened this issue 4 years ago • 16 comments

The command annotation is thought as a complement or replacement for the present command annotate.

Synopsis

$ timew annotation set [<ID>...] <ANNOTATION>
$ timew annotation add [<ID>...] <ADDENDUM>
$ timew annotation show [<ID>...]

Description

  • Annotations can be set with the set subcommand:
    $ timew start FOO
    $ timew annotation set @1 <ANNOTATION>
    Annotated @1 with "<ANNOTATION>"
    
    This would overwrite any present annotation.
  • Deleting an annotation is done by setting it to an empty string
    ...
    $ timew annotation set @1 ''
    Removed annotation from @1
    
  • The annotation command will have an implicit @1, i.e. if no <ID> is given, it will operate on the currently open interval if present:
    $ timew start FOO
    $ timew annotation set <ANNOTATION>
    Annotated @1 with "<ANNOTATION>"
    
    $ timew track FOO <start> - <end>
    $ timew annotation set <ANNOTATION>
    At least one ID must be specified. See 'timew help annotation'.
    
  • To add to an annotation, the add subcommand can be used:
    $ timew start FOO
    $ timew annotation set @1 <ANNOTATION>
    ...
    $ timew annotation add @1 <ADDENDUM>
    Annotated @1 with "<ANNOTATION> <ADDENDUM>"
    
    The <ADDENDUM> is appended with a separating space. If no annotation is present, the behaviour of add is equal to set.
  • Annotations can be viewed with the show subcommand:
    $ timew track FOO BAZ <S1> - <E1>
    $ timew start BAR <S2>
    $ timew annotate @2 <ANNOTATION>
    ...
    $ timew annotation show @1 @2
    Interval @2 [<from> -  <to>] (FOO, BAZ) is annotated with
       <ANNOTATION>
    Interval @1 [<from> - ...] (BAR) is not annotated.
    

The command annotate would then either become a shortcut for annotation set or be replaced by it.

Also see updates to this in https://github.com/GothenburgBitFactory/timewarrior/issues/374#issuecomment-678317512

lauft avatar Aug 21 '20 12:08 lauft

Great idea! I have a few thoughts:

Why not also offer unset instead of the rather cumbersome set '' for deleting existing annotations?

Also, I think the new command should not complement, but replace the annotate command. What about making set the default command so that instead of

$ timew start FOO
$ timew annotation set <ANNOTATION>

one could also write

$ timew start FOO
$ timew annotation <ANNOTATION>

while still offering add and show as options (and set, if one prefers to be explicit)? In this way, annotation would offer the same functionality as annotate and in a way simply extend its functionality, so that annotate itself could become deprecated.

What I would like in addition (but I admit that it's somewhat beside the point of this issue) is that the summary :annotation command does not only display the first 15 characters of the annotation, but more. This could be done by extending the width of the table so that it stretches all across the terminal window width to display as many characters as possible (although I do not know whether it is possible – and if, how easy it is – to obtain that option). Alternatively, the number of annotation characters to display could be a configurable property.

goetzd avatar Aug 21 '20 13:08 goetzd

@goetzd

Why not also offer unset instead of the rather cumbersome set '' for deleting existing annotations?

I tried to keep the command set rather small, but I agree we can add it as a shortcut to set with an empty string:

  • Deleting an annotation is done with the unset subcommand
    ...
    $ timew annotation unset @1
    Removed annotation from @1
    
    or by setting it to an empty string:
    ...
    $ timew annotation set @1 '' 
    Removed annotation from @1
    

Also, I think the new command should not complement, but replace the annotate command. What about making set the default command ...

I originally had the idea of making show the default command, but I agree that set seems a better choice.

  • The set subcommand is the default, so it may be omitted:
    $ timew annotation <ID> <ANNOTATION>
    
    is equal to
    $ timew annotation set <ID> <ANNOTATION>
    

lauft avatar Aug 21 '20 14:08 lauft

Exciting to see this proposal thank you!

Having set be default behavior sounds great to me.

Could add be built into set? It'd be great if timew annotation "stuff" and then timew annotation "more stuff" could result in an annotation like "stuff | more stuff" or just "stuff more stuff" (with a space inserted) without having to specify the add command. Can this be built into the set command?

ahillio avatar Aug 21 '20 15:08 ahillio

Bonus points would be to be able to edit the existing annotation in vim/emacs/etc/default.

My workflow includes adding notes to a time annotation as I do work, I do more work and add more notes. So if I've had the timer running for multiple hours there's a good chance I'll want to edit/cleanup the annotation.

Could this be done similar to how taskwarrior can do task 17 edit?

ahillio avatar Aug 21 '20 15:08 ahillio

Could add be built into set?

I wonder whether the default sub-command for annotations could be made configurable. I get the feeling that there might be "Team set" and a "Team add" out there... 🤔

Bonus points would be to be able to edit the existing annotation in vim/emacs/etc/default.

A timew annotation edit may not be part of the first implementation, but it's definitely an idea worth considering.

lauft avatar Aug 21 '20 16:08 lauft

What do you mean by "Team set" and "Team add"? That's in response to my suggestion of having set automatically append on to an existing annotation?

ahillio avatar Aug 22 '20 02:08 ahillio

I mean that I suspect two user groups: One which does prefer set, the other add to be the default subcommand. 😉

This "conflict" could be solved by making the default sub-command configurable. If this is possible without large trade-offs, I would opt for this solution than having to decide for one option for all.

lauft avatar Aug 22 '20 08:08 lauft

Oh! I see what you're saying now. I was trying to describe something different...

No add command. Instead of having a separate command for adding to an existing annotation, the behavior of set command has a condition to check for existing annotation: if none exists create one, and if one does exist append to it automatically. Instead of having an add command, could the set command be smart enough to create/append on its own?

ahillio avatar Aug 22 '20 13:08 ahillio

What you describe is the proposed behaviour of add.

$ timew start FOO
$ timew annotation add <ANNOTATION>
Annotated @1 with "<ANNOTATION>"
$ timew annotation add <ADDENDUM>
Annotated @1 with "<ANNOTATION> <ADDENDUM>"

lauft avatar Aug 22 '20 13:08 lauft

Oh... so then the intended purpose of set would be to replace an existing annotation?

ahillio avatar Aug 22 '20 14:08 ahillio

This looks great! However, a few suggestions (I apologize for the bikeshedding yet again...):

  • Why do we need an entirely new annotation sub-command for this? Can't this just replace the existing annotate?
  • I believe annotation add should be the default, not annotation set, for two reasons: 1) add already has the semantics of "setting" the annotation if no annotation is present (according to the original description here: https://github.com/GothenburgBitFactory/timewarrior/issues/374#issue-683539839), and 2) the issue in #365 was specifically about annotate NOT appending to the existing annotation, and how this can be confusing for users. (And @lauft - I believe that if annotation add becomes the default there won't be a need for any configuration knob to select the default -- add seems to do everything everybody wants?)
  • Personally, I find the annotation set '' to remove an annotation to be undesirable for three reasons: 1) it just seems hacky to me, 2) it's clear we'll already need at least two sub-sub-commands (more on this below) so adding an explicit delete or remove or whatever seems reasonable, and 3) the risk of scripts innocently or accidentally calling annotation set '' either because of a typo in a variable name or because something else isn't setting an environment variable (or setting an env var to an empty string, etc.) seems too high for such a potentially destructive behavior; adding an explicit annotation delete (or annotation remove or whatever) seems much safer from a scriptability standpoint, imo.

Food for thought: if annotation add became the default and there was a corresponding annotation delete (or annotation remove) then there wouldn't be a need for annotation set at all. The same behavior could be achieved by explicitly deleting the existing annotation and then adding a new annotation. Personally, I think I would still like to have an explicit annotation set if for no other reason than because it's behavior mimics the existing annotate sub-command's, but if there's concern about having too many sub-sub-commands, or this command doing too much, or the code getting too complicated, I did just want to point out that add and delete are the minimum required to support all known use-cases so far: create a new annotation, append to an existing annotation, reset an existing annotation.

(I'm deliberately ignore annotation show in my analysis above since it's inclusion & behavior both seem uncontroversial)

sl4mmy avatar Aug 22 '20 15:08 sl4mmy

If annotation set was default, then there would be times when a user would loose data because they forget or don't realize how the software works.... Unsuspecting user would:

$ timew annotation "first set of work."
$ timew annotation "second set of work"
$ timew annotation show
// second set of work

unsuspecting user then complains "hey, where'd my first set of work go?"

So in that sense annotation add would be a more conservative default.

ahillio avatar Aug 22 '20 16:08 ahillio

Thanks to everyone for their suggestions, I will consider them while reworking the draft. 👍🏻

lauft avatar Aug 22 '20 21:08 lauft

Just adding another use case to consider.

I would like an timew annotation edit @2 where the annotation is saved to a temporary file, and editor is popped open, and then when the editor closes, the temporary file is read in as the new annotation.

Granted, with timew annotation show and timew annotation set I can accomplish the same thing with a wrapper script so not sure if this would be something core timewarrior should handle.

sruffell avatar Aug 24 '20 09:08 sruffell

Just adding another use case to consider. I would like an timew annotation edit @2 ...

@sruffell 👍🏻 See https://github.com/GothenburgBitFactory/timewarrior/issues/374#issuecomment-678355974 and https://github.com/GothenburgBitFactory/timewarrior/issues/374#issuecomment-678380013, this is already considered.

lauft avatar Aug 25 '20 07:08 lauft

As for the default commands:

timew annotation <ID>

Should invoke show, i.e. show the annotations (the ID could even be optional, in which case it would be @1 by default)

On the other hand, this should invoke add:

timew annotation <ID> <ANNOTATION>

I don't see why we would have "Team set" - overriding an annotation doesn't seem like a common use-case to me?

xeruf avatar Sep 08 '20 11:09 xeruf