From 9849f196cbd1ea173afb3fddc9fea91a31863494 Mon Sep 17 00:00:00 2001 From: Simon Michael Date: Mon, 1 Sep 2025 08:11:47 +0100 Subject: [PATCH] fix:timeclock: process in parse order; fully support overlapping sessions [#2417] We no longer attempt to process timeclock entries in time order - that was a wrong requirement, probably given by me, that can't work. Now we just process them in parse order. This plus a little tweaking of error checking fixes several ordering bugs with overlapping sessions and also allows same-named overlapping sessions. More cleanup will follow. More testing might show that --old-timeclock is no longer needed. --- hledger-lib/Hledger/Data/Timeclock.hs | 76 +++++++++++++------------ hledger/hledger.m4.md | 14 ++--- hledger/test/errors/tcclockouttime.test | 7 +-- hledger/test/timeclock.test | 29 ++++++++-- 4 files changed, 72 insertions(+), 54 deletions(-) diff --git a/hledger-lib/Hledger/Data/Timeclock.hs b/hledger-lib/Hledger/Data/Timeclock.hs index bfc979d85..46f076c82 100644 --- a/hledger-lib/Hledger/Data/Timeclock.hs +++ b/hledger-lib/Hledger/Data/Timeclock.hs @@ -79,8 +79,8 @@ timeclockToTransactionsOld :: LocalTime -> [TimeclockEntry] -> [Transaction] timeclockToTransactionsOld _ [] = [] timeclockToTransactionsOld now [i] | tlcode i /= In = errorExpectedCodeButGot In i - | odate > idate = entryFromTimeclockInOut i o' : timeclockToTransactionsOld now [i',o] - | otherwise = [entryFromTimeclockInOut i o] + | odate > idate = entryFromTimeclockInOut True i o' : timeclockToTransactionsOld now [i',o] + | otherwise = [entryFromTimeclockInOut True i o] where o = TimeclockEntry (tlsourcepos i) Out end "" "" "" [] end = if itime > now then itime else now @@ -91,8 +91,8 @@ timeclockToTransactionsOld now [i] timeclockToTransactionsOld now (i:o:rest) | tlcode i /= In = errorExpectedCodeButGot In i | tlcode o /= Out = errorExpectedCodeButGot Out o - | odate > idate = entryFromTimeclockInOut i o' : timeclockToTransactionsOld now (i':o:rest) - | otherwise = entryFromTimeclockInOut i o : timeclockToTransactionsOld now rest + | odate > idate = entryFromTimeclockInOut True i o' : timeclockToTransactionsOld now (i':o:rest) + | otherwise = entryFromTimeclockInOut True i o : timeclockToTransactionsOld now rest where (itime,otime) = (tldatetime i,tldatetime o) (idate,odate) = (localDay itime,localDay otime) @@ -105,16 +105,18 @@ timeclockToTransactionsOld now (i:o:rest) -- It allows concurrent clocked-in sessions (though not with the same account name), -- and clock-in/clock-out entries in any order. -- --- Entries are processed in time order, then (for entries with the same time) in parse order. --- When there is no clockout, one is added with the provided current time. +-- Entries are processed in parse order. -- Sessions crossing midnight are split into days to give accurate per-day totals. +-- At the end, any sessions with no clockout get an implicit clockout with the provided "now" time. -- If any entries cannot be paired as expected, an error is raised. -- timeclockToTransactions :: LocalTime -> [TimeclockEntry] -> [Transaction] -timeclockToTransactions now entries = transactions +timeclockToTransactions now entries0 = transactions where - sessions = dbg6 "sessions" $ pairClockEntries (sortTimeClockEntries entries) [] [] - transactionsFromSession s = entryFromTimeclockInOut (in' s) (out s) + -- don't sort by time, it messes things up; just reverse to get the parsed order + entries = dbg7 "timeclock entries" $ reverse entries0 + sessions = dbg6 "sessions" $ pairClockEntries entries [] [] + transactionsFromSession s = entryFromTimeclockInOut False (in' s) (out s) -- If any "in" sessions are in the future, then set their out time to the initial time outtime te = max now (tldatetime te) createout te = TimeclockEntry (tlsourcepos te) Out (outtime te) (tlaccount te) "" "" [] @@ -122,11 +124,6 @@ timeclockToTransactions now entries = transactions stillopen = dbg6 "stillopen" $ pairClockEntries ((active sessions) <> outs) [] [] transactions = map transactionsFromSession $ sortBy (\s1 s2 -> compare (in' s1) (in' s2)) (completed sessions ++ completed stillopen) - -- | Sort timeclock entries first by date and time (with time zone ignored as usual), then by file position. - -- Ie, sort by time, but preserve the parse order of entries with the same time. - sortTimeClockEntries :: [TimeclockEntry] -> [TimeclockEntry] - sortTimeClockEntries = sortBy (\e1 e2 -> compare (tldatetime e1, tlsourcepos e1) (tldatetime e2, tlsourcepos e2)) - -- | Assuming that entries have been sorted, we go through each time log entry. -- We collect all of the "i" in the list "actives," and each time we encounter -- an "o," we look for the corresponding "i" in actives. @@ -161,16 +158,6 @@ timeclockToTransactions now entries = transactions <> [ "Overlapping sessions with the same account name are not supported." ] -- XXX better to show full session(s) -- <> map T.show (filter ((`elem` activesinthisacct).in') sessions) - where - makeTimeClockErrorExcerpt :: TimeclockEntry -> T.Text -> T.Text - makeTimeClockErrorExcerpt e@TimeclockEntry{tlsourcepos=pos} msg = T.unlines [ - T.pack (sourcePosPretty pos) <> ":" - ,l <> " | " <> T.show e - -- ,T.replicate (T.length l) " " <> " |" -- <> T.replicate c " " <> "^") - ] <> msg - where - l = T.show $ unPos $ sourceLine $ tlsourcepos e - -- c = unPos $ sourceColumn $ tlsourcepos e -- | Find the relevant clockin in the actives list that should be paired with this clockout. -- If there is a session that has the same account name, then use that. @@ -208,12 +195,22 @@ errorExpectedCodeButGot expected actual = error' $ printf l = show $ unPos $ sourceLine $ tlsourcepos actual c = unPos $ sourceColumn $ tlsourcepos actual +makeTimeClockErrorExcerpt :: TimeclockEntry -> T.Text -> T.Text +makeTimeClockErrorExcerpt e@TimeclockEntry{tlsourcepos=pos} msg = T.unlines [ + T.pack (sourcePosPretty pos) <> ":" + ,l <> " | " <> T.show e + -- ,T.replicate (T.length l) " " <> " |" -- <> T.replicate c " " <> "^") + ] <> msg + where + l = T.show $ unPos $ sourceLine $ tlsourcepos e + -- c = unPos $ sourceColumn $ tlsourcepos e + -- | Convert a timeclock clockin and clockout entry to an equivalent journal -- transaction, representing the time expenditure. Note this entry is not balanced, -- since we omit the \"assets:time\" transaction for simpler output. -entryFromTimeclockInOut :: TimeclockEntry -> TimeclockEntry -> Transaction -entryFromTimeclockInOut i o - | otime >= itime = t +entryFromTimeclockInOut :: Bool -> TimeclockEntry -> TimeclockEntry -> Transaction +entryFromTimeclockInOut requiretimeordered i o + | not requiretimeordered || otime >= itime = t | otherwise = -- Clockout time earlier than clockin is an error. -- (Clockin earlier than preceding clockin/clockout is allowed.) @@ -260,8 +257,13 @@ entryFromTimeclockInOut i o -- since otherwise it will often have large recurring decimal parts which (since 1.21) -- print would display all 255 digits of. timeclock amounts have one second resolution, -- so two decimal places is precise enough (#1527). - amt = mixedAmount $ setAmountInternalPrecision 2 $ hrs hours - ps = [posting{paccount=acctname, pamount=amt, ptype=VirtualPosting, ptransaction=Just t}] + amt = case mixedAmount $ setAmountInternalPrecision 2 $ hrs hours of + a | not $ a < 0 -> a + _ -> error' $ printf + "%s%s:\nThis clockout is earlier than the clockin." + (makeTimeClockErrorExcerpt i "") + (makeTimeClockErrorExcerpt o "") + ps = [posting{paccount=acctname, pamount=amt, ptype=VirtualPosting, ptransaction=Just t}] -- tests @@ -292,13 +294,13 @@ tests_Timeclock = testGroup "Timeclock" [ step "use the clockin time for auto-clockout if it's in the future" txndescs [clockin future "" "" "" []] @?= [printf "%s-%s" futurestr futurestr] step "multiple open sessions" - txndescs - [ clockin (mktime today "00:00:00") "a" "" "" [], - clockin (mktime today "01:00:00") "b" "" "" [], - clockin (mktime today "02:00:00") "c" "" "" [], - clockout (mktime today "03:00:00") "b" "" "" [], - clockout (mktime today "04:00:00") "a" "" "" [], - clockout (mktime today "05:00:00") "c" "" "" [] - ] + txndescs (reverse [ + clockin (mktime today "00:00:00") "a" "" "" [], + clockin (mktime today "01:00:00") "b" "" "" [], + clockin (mktime today "02:00:00") "c" "" "" [], + clockout (mktime today "03:00:00") "b" "" "" [], + clockout (mktime today "04:00:00") "a" "" "" [], + clockout (mktime today "05:00:00") "c" "" "" [] + ]) @?= ["00:00-04:00", "01:00-03:00", "02:00-05:00"] ] diff --git a/hledger/hledger.m4.md b/hledger/hledger.m4.md index b911193a7..aa56bc250 100644 --- a/hledger/hledger.m4.md +++ b/hledger/hledger.m4.md @@ -4720,9 +4720,9 @@ of [timeclock.el](http://www.emacswiki.org/emacs/TimeClock). As with [Ledger](http://ledger-cli.org/3.0/doc/ledger3.html#Time-Keeping), hledger's timeclock format is a subset/variant of timeclock.el's. -Note, hledger's timeclock format was made more robust in hledger 1.43 and 1.50. +hledger's timeclock format was updated in hledger 1.43 and 1.50. If your old time logs are rejected, you should adapt them to modern hledger; -but for now you can also restore the pre-1.43 behaviour with the `--old-timeclock` flag. +for now, you can restore the pre-1.43 behaviour with the `--old-timeclock` flag. Here the timeclock format in hledger 1.50+: @@ -4768,8 +4768,11 @@ $ hledger -f a.timeclock print ``` Clock-ins and clock-outs are matched by their account/session name. -If a clock-outs does not specify a name, the most recent unclosed clock-in is closed. -Also, sessions spanning more than one day are automatically split at day boundaries. +If a clock-out does not specify a name, the most recent unclosed clock-in is closed. +You can have multiple sessions active simultaneously. +Entries are processed in the order they are parsed. +Sessions spanning more than one day are automatically split at day boundaries. + Eg, the following time log: ```timeclock @@ -4804,9 +4807,6 @@ $ hledger -f t.timeclock print ``` -Note, you can have overlapping sessions (multiple sessions open simultaneously), but they must have different account names. -Overlapping sessions with the same account name are currently not supported currently. - Here is a [sample.timeclock](https://raw.github.com/simonmichael/hledger/master/examples/sample.timeclock) to download and some queries to try: diff --git a/hledger/test/errors/tcclockouttime.test b/hledger/test/errors/tcclockouttime.test index ac31612f3..6739d9d52 100644 --- a/hledger/test/errors/tcclockouttime.test +++ b/hledger/test/errors/tcclockouttime.test @@ -1,8 +1,3 @@ $$$ hledger check -f tcclockouttime.timeclock ->>>2 /Error: .*tcclockouttime.timeclock:5:1: -5 \| o 2022-01-01 00:00:00 - \| \^ - -Could not find previous clockin to match this clockout. -/ +>>>2 /This clockout is earlier than the clockin./ >>>= 1 diff --git a/hledger/test/timeclock.test b/hledger/test/timeclock.test index 36a6ae766..90926a4cf 100644 --- a/hledger/test/timeclock.test +++ b/hledger/test/timeclock.test @@ -192,7 +192,7 @@ $ hledger -f timeclock:- print >= -# ** 14. Sessions with the same account name may not overlap (#2417). +# ** 14. Sessions can overlap, even with the same account name (#2417). < i 2024-01-01 13:00:00 a o 2024-01-01 15:00:00 @@ -200,10 +200,31 @@ i 2024-01-01 14:00:00 a o 2024-01-01 15:00:00 $ hledger -f timeclock:- print ->2 /Overlapping sessions with the same account name are not supported./ ->=!0 +2024-01-01 * 13:00-15:00 + (a) 2.00h -# ** 15. --old-timeclock also affects included files. +2024-01-01 * 14:00-15:00 + (a) 1.00h + +>= + +# ** 15. These staggered overlapping sessions are read correctly. +< +i 2000-01-01 09:00:00 a +o 2000-01-01 12:00:00 +i 2000-01-01 10:00:00 b +o 2000-01-01 13:00:00 + +$ hledger -f timeclock:- print +2000-01-01 * 09:00-12:00 + (a) 3.00h + +2000-01-01 * 10:00-13:00 + (b) 3.00h + +>= + +# ** 16. --old-timeclock also affects included files. < include needs-old.timeclock