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

describing times without minutes results in bad times #37

Closed
3 tasks
skyfaller opened this issue Jun 27, 2020 · 6 comments · Fixed by #38
Closed
3 tasks

describing times without minutes results in bad times #37

skyfaller opened this issue Jun 27, 2020 · 6 comments · Fixed by #38
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@skyfaller
Copy link

skyfaller commented Jun 27, 2020

This is bad:

nelson@Valence ~> rtw track 9 - 11 shower etc.
Recorded shower etc.
Started 1969-12-31T18:59:59
Ended   1969-12-31T18:59:59
Total   00:00:00

It should have the same result as this:

nelson@Valence ~> rtw track 9:00 - 11:00 shower etc.
Recorded shower etc.
Started 2020-06-27T09:00:00
Ended   2020-06-27T11:00:00
Total   02:00:00

Also, I can't figure out how to delete that spurious date from within rtw, it didn't show up in the summary for the week because it's from 1969:

nelson@Valence ~> rtw summary --week
tea               2020-06-23T09:25:05 2020-06-23T10:10:29 00:45:24
talk with parents 2020-06-23T13:00:00 2020-06-23T13:50:00 00:50:00
guitar lesson     2020-06-23T14:30:00 2020-06-23T15:30:00 01:00:00
protest           2020-06-23T16:00:00 2020-06-23T17:12:59 01:12:59
shower etc.       2020-06-26T06:45:00 2020-06-26T08:45:00 02:00:00
tea               2020-06-26T09:23:51 2020-06-26T10:35:03 01:11:12
OJ                2020-06-26T10:45:11 2020-06-26T12:31:31 01:46:20
tea               2020-06-26T17:42:07 2020-06-26T18:29:25 00:47:18

So I don't know how to get the id number to delete. I had to manually edit the JSON file to get rid of it. (Thanks again for using a standardized "plaintext" file format instead of some weird database, it makes it much easier to fix things when the program goofs.)

Some possible solutions:

  • Clarify the exact requirements for input formats in documentation
  • Don't allow the user to enter in data rtw doesn't understand
  • Correctly parse hours without minutes attached
@PicoJr
Copy link
Owner

PicoJr commented Jun 27, 2020

Hello, date times are parsed using chrono-english crate.

In the project README it shows:

$ alias p='cargo run --quiet --example parse-date --'
$ p 'next April'
base Wed Mar 14 20:10:37 2018 +0200
calc Sun Apr  1 00:00:00 2018 +0200
$ p '20/03/18 12:04'
base Wed Mar 14 20:12:44 2018 +0200
calc Tue Mar 20 12:04:00 2018 +0200
$ p '9/11/01' --american
base Wed Mar 14 20:13:08 2018 +0200
calc Tue Sep 11 00:00:00 2001 +0200
$ p 'next fri 8pm' '2018-03-14'
base Wed Mar 14 00:00:00 2018 +0200
calc Fri Mar 16 20:00:00 2018 +0200

On my machine:

p 9 outputs:

base Sun Jun 28 00:29:38 2020 +0200 (2020-06-28T00:29:38.879676215+02:00)
calc Thu Jan  1 00:00:00 0009 +0009 (0009-01-01T00:00:00+00:09)

The calculated value is surprising ^^' it might be a bug with the chrono crate, I might also have gotten something wrong when calling parse_date_string

I need to check that when I have some time =)

@skyfaller
Copy link
Author

Here's another failure:

nelson@Valence ~/D/G/linode-caddy (master)> rtw track 8am - 10am shower etc.
Recorded
Started 2020-06-28T08:00:00
Ended   2020-06-28T10:00:00
Total   02:00:00

I'm not sure why it got the time right but it ate the description of the activity. Adding quotes doesn't help:

nelson@Valence ~/D/G/linode-caddy (master)> rtw track 8am - 10am "shower etc."
Recorded
Started 2020-06-28T08:00:00
Ended   2020-06-28T10:00:00
Total   02:00:00

It should look like this:

nelson@Valence ~/D/G/linode-caddy (master)> rtw track 8:00 - 10:00 shower etc.
Recorded shower etc.
Started 2020-06-28T08:00:00
Ended   2020-06-28T10:00:00
Total   02:00:00

@PicoJr
Copy link
Owner

PicoJr commented Jun 28, 2020

The issue is that parse_date_string("10am shower etc.", Local::now(), Dialect::Uk).is_ok() evaluates to true.

Here's how rtw parses track 8am - 10am shower etc.:

  1. split left and right side of -: (8am, 10am shower etc.)
  2. start time is 8am (easy)
  3. try parsing 10am shower etc. as a time (it should fail but unfortunately it does not)
  4. try parsing 10 am shower (+etc.) as a time (it should fail...)
  5. try parsing 10am (+shower etc.) as a time (it should be ok)
  6. stop time is 10am activity is shower etc.

Here's the piece of code that does just that:

rtw/src/cli_helper.rs

Lines 10 to 23 in c2765cc

// 09:00 foo -> (09:00, foo)
// foo -> (Now, foo)
// last friday 8pm foo -> (last friday 8pm, foo)
fn split_time_clue_from_tags(tokens: &[String], clock: &dyn Clock) -> (Time, Tags) {
for at in (0..=tokens.len()).rev() {
let (possibly_time_clue, possibly_tags) = tokens.split_at(at);
let possibly_time_clue_joined: &str = &possibly_time_clue.join(" ");
if TimeTools::is_time(possibly_time_clue_joined) {
let time = TimeTools::time_from_str(possibly_time_clue_joined, clock).unwrap();
return (time, possibly_tags.to_vec());
}
}
(Time::Now, tokens.to_vec())
}

I have no clue how to solve this for now...

So far this issue references 2 bugs

numbers are interpreted as year instead of hours

❯ rtw start 9 foo
Tracking foo
Started  0009-01-01T00:00:00

wrong parsing when supplying am|pm

❯ rtw track 8am - 10am shower
Recorded 
Started 2020-06-28T08:00:00
Ended   2020-06-28T10:00:00
Total   02:00:00

Fix contribution would be appreciated =)

@PicoJr PicoJr added bug Something isn't working help wanted Extra attention is needed labels Jun 28, 2020
@skyfaller
Copy link
Author

Sorry, should I split these into separate issues? It wasn't clear to me how related they were.

@PicoJr
Copy link
Owner

PicoJr commented Jul 2, 2020

Sorry, should I split these into separate issues? It wasn't clear to me how related they were.

It's fine, I'm working on it but the fix won't land anytime soon ;)

PicoJr added a commit that referenced this issue Jul 5, 2020
@PicoJr
Copy link
Owner

PicoJr commented Jul 5, 2020

I wrote htp to parse time clues for rtw.

It fixes both issues above but it does not support as many format as chrono-english.

Once I'm confident rtw still works fine with htp, I'll release it as a new major release.

@PicoJr PicoJr closed this as completed in #38 Jul 5, 2020
PicoJr added a commit that referenced this issue Jul 5, 2020
fix #37, replace chrono-english with htp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants