nomeata / arbtt

arbtt, the automatic rule-based time-tracker
http://arbtt.nomeata.de/
GNU General Public License v2.0
314 stars 36 forks source link

Migrate from deprecated runGetState to runGetOrFail; Fix parsing of empty logfiles #150

Closed liskin closed 2 years ago

liskin commented 2 years ago

Migrate from deprecated runGetState to runGetOrFail

runGetState has been deprecated since binary-0.6.0.0 (2012).

This also happens to fix error positions as runGetState ignores the off parameter when reporting errors. Given a logfile with just the header but no entries, the old code reports:

arbtt-dump: Data.Binary.Get.runGetState at position 0: not enough bytes
CallStack (from HasCallStack):
  error, called at libraries/binary/src/Data/Binary/Get.hs:312:5 in binary-0.8.7.0:Data.Binary.Get

The new correctly reports:

arbtt-dump: Timelog parse error at 17: not enough bytes
CallStack (from HasCallStack):
  error, called at src/TimeLog.hs:150:13 in main:TimeLog

Fix parsing of empty logfiles

arbtt-import will happily create a logfile with just a header and no entries, so arbtt-{dump,stats} shouldn't crash when parsing it.

liskin commented 2 years ago

How confident are you?

What do you mean?

nomeata commented 2 years ago

I mean, is this a “this might work, wdyt” PR, or a “i'd ship this, please merge”? In the latter case I'll just press the button, in the former I'll have to sit down and think :-)

liskin commented 2 years ago

Oh, I see. I think I'd probably ship this. The only concern is the switch from catch to Either and the associated switch from partial functions to MonadFail: I tried to hunt all the places where this needs changing but may have missed some. Not entirely sure if it's worth wasting more time on that, worst case is arbtt-recover will crash and we'll fix it later, I guess? :-)

liskin commented 2 years ago

BTW, is there a good reason why you're squashing my commits? :-(

nomeata commented 2 years ago

Oh, sorry, habit from other repositories. If you haven't pulled master yet I can fix with a force push?

liskin commented 2 years ago

Even if I had, I know enough git to deal with that ;-)

nomeata commented 2 years ago

Fixed

nomeata commented 2 years ago

No need to restore the branch, I took it off GitHub's refs/pull/150/head :-)

liskin commented 2 years ago

Cool, thanks! :-)