GothenburgBitFactory / taskwarrior

Taskwarrior - Command line Task Management
https://taskwarrior.org
MIT License
4.52k stars 314 forks source link

"task calc 12h + 25m + 30s" gives 750+ days #2898

Open smemsh opened 2 years ago

smemsh commented 2 years ago

with taskwarrior v2.6.2:

$ src/task calc 12h + 25m + 30s
P750DT12H30S

discrepant with https://taskwarrior.org/docs/commands/calc/ section "Durations" where it claims the output should be:

$ task calc 12h + 25m +30s
PT12H25mPT30S

(I tried with and without the space after second +, present on web site but makes no difference) (not sure if libshared is the right place for this issue?)

tbabej commented 2 years ago

Looks like m gets interpreted as month, so that's where 750 (=25x30 days) comes from. Using min instead of m yields the expected result:

task calc 12h + 25min + 30s
PT12H25M30S

Perhaps we should try to be case-sensitive in the parsing of the intervals, where M would mean month and m a minute.

smemsh commented 2 years ago

Ah so that's where it came from. Actually I noticed even the supposed output on that page was not being parsed correctly even when it was written, it says:

$ task calc 12h + 25m +30s
PT12H25mPT30S

Note the m in the middle, probably unchanged from the input m. It's giving two separate time durations, as denoted by two PT. looks like it was seeing three expressions: (1) the interval 12h + 25, (2) the letter m, and then (3) 30s. So probably this was never correct even in prior versions.

Perhaps we should try to be case-sensitive in the parsing of the intervals, where M would mean month and m a minute.

I do think most people would assume a single m was a minute rather than a month. Also note that mo, mos, month and months already are availble to mean 30 days (but mon or mons does not work).

Actually, timew-durations(7) shows:

minutes, minute, mins, min
monthly, months, month, mnths, mths, mth, mos, mo, m

Also we should consider that ISO8601 durations use capital M for minute, ie the format being:

P<date>T<days>D<hours>H<minutes>M<seconds>S

I would vote for m changed to mean minutes, rather than months, because that's what the obvious meaning is, but it would be a breaking change. I like it better, but admittedly it's subjective.

Perhaps just the docs at that url should be changed to use correct syntax. Or we should wait to see if there's any other comments...

ar1428 commented 2 years ago

I don't think this should be changed. Task dates are far more likely to be measured in days, weeks, months vs seconds, minutes, or even hours. I can't remember ever needing to add a task with a due date in X minutes time.

smemsh commented 2 years ago

this is not just for setting for due dates, it's for any operation where durations are used in any context (both task and time -warrior), they are all being parsed by this libshared code. I frequently use minutes and seconds and do "task calc ... " in subshells to make start/end times for example. it seems really strange to me that hms stands for hour, month, second rather than hour, minute, second, I think would be the normal expectation (carried over from other contexts too, unrelated to task warrior, where it would be the typical expectation).

That said you're the only one that voted besides me and it's against the change so hopefully we'll get some other votes!

smemsh commented 2 years ago

pressed wrong button didn't mean to close

ar1428 commented 2 years ago

@smemsh I've not needed to use timewarrior, so wasn't particularly aware of that context.

Some other ideas:

djmitche commented 6 months ago

This seems to be a common error. I improved the documentation a little a few months back, so hopefully that helps?

smemsh commented 6 months ago

@djmitche closing doesn't address this ticket, question is, should 'm' be changed to mean minutes, based on user expectation, possibly with big-'M' to mean months.

dathanb commented 6 months ago

I never schedule tasks at minute resolution, so the "month" default is more natural for me -- and, I expect (though I could be wrong), most Taskwarrior users.

It's just my opinion, obviously, but I don't think the gain for folks that do use minute resolutions is large enough to justify inflicting a breaking change on everyone else. A config to pick between them could make sense if someone wants to work on it, but it's not a feature I'd vote for.

djmitche commented 6 months ago

I agree that breaking this isn't a clear enough win to justify -- this value "makes sense" for lots of people, just as minutes "makes sense" for lots of other people.

It would be unusual to allow config to control this -- TW does not have other configs I can think of that affect command-line parsing. And it takes config on the command-line, so there's a bit of a cyclical dependency there!

By far I think the more common use of durations in Taskwarrior is wait or due or other task attributes, which typically would not use addition (I have no idea what task calc is for!). So I think a warning on expressions involving both "small" and "large" deltas wouldn't help.

At the moment, I think the best resolution is just to type min instead of m to mean minutes. I ran into this at some point in my TW career: I occasionally want to put a task out-of-mind just long enough for tests to run or sometihng, and task NN wait:30m seems natural but the effect is a little longer than desired. I've gotten in the habit of using task NN wait:30min instead.

That said, if you'd like to continue to brainstorm and maybe implement a solution to this issue, I'm happy to re-open.

dathanb commented 6 months ago

I agree with most of that, except for

By far I think the more common use of durations in Taskwarrior is wait or due or other task attributes, which typically would not use addition

I do think that's the most common use case, but I will frequently do task NN mod wait:Friday+1w, or task NN mod due:Friday+1m, so at least for me addition isn't uncommon for those two attributes.

djmitche commented 6 months ago

Huh, TIL Friday+1w!

smemsh commented 6 months ago

if there are comments already from those that prefer the existing meaning, it doesn't make sense to change it. Making it configurable is out of scope for me. I'll adjust, thanks.

tbabej commented 6 months ago

Maybe one thing that we could do is to emit a warning when unusually large number of months is used in a computation. I would argue no (human) typically thinks of time intervals spanning more than 12 months (it's more natural to say 1y and 8 months than 20 months, say), we could emit a warning when a datetime computation happens to encounter an term of more than 12m, i.e the following would occur:

$ task add Finish the presentation due:Friday+8h+30m
Warning: '30m' interpreted as 30 months. If you wished to specify minutes, please use '30min' instead.
Task 8 added.
tbabej commented 6 months ago

Actually I see @ar1428 suggesting this earlier in the thread, which might be even a better trigger for the warning:

Attempt to detect and warn the user of likely incorrect usage, e.g. hours and months together