git-date parse draft #498

svetli-n · 2022-08-23T20:55:44Z

Tasks

make parse(input, Option<currenttime>) pure, allowing the current time to be passed
relative time i.e. 2 seconds ago
gix rev parse '@{1979-02-26 18:30:00}' -e - should use a nicer format when displaying the parsed time

git-date/src/parse.rs

svetli-n · 2022-08-23T20:58:35Z

Hey @Byron can I get early feedback, thanks!

Byron

Thanks for the early feedback request, much appreciated!

The structure of the parse module can scale to all the sub-parsers you might be writing, nice!

Something that I'd definitely add to the test-suite is a git baseline for absolute dates. One way I found to trigger git's date parsing is git -c a.d=1979-02-26 config --expiry-date a.d, which can be used in a fixture script to produce a baseline like here. This file can then be parsed back for quick comparisons like here.

With that you can validate that the date parsed is the same, and if not you could also mark it as such so it's clear we don't expect a perfect match. The reason is that I don't trust git parses dates correctly at all or handles them correctly, so time is probably better there.

However, with the hand-implemented parsers, there I would expect a perfect match against git. Ideally, each input tested in git-date is also run against the git baseline, and you can setup your test suite to enforce this. Note that it's not viable to call git each time, the baseline should be cached with a fixture script like in the example.

A lot to digest, I can elaborate if there are questions either here or in the session. Good luck!

git-date/src/parse.rs

Byron · 2022-08-24T01:53:15Z

git-date/src/parse.rs

 use crate::Time;
+use time::{Date, OffsetDateTime};

 #[allow(missing_docs)]
 pub fn parse(input: &str) -> Option<Time> {


This should return Result<Time, Error> with Error covering typical cases.

Byron · 2022-08-24T01:53:54Z

git-date/src/parse.rs

-        None
+        return if let Ok(val) = Date::parse(input, SHORT) {
+            let val = val.with_hms(0, 0, 0).expect("date is in range").assume_utc();
+            Some(Time::new(val.unix_timestamp() as u32, val.offset().whole_seconds()))


Instead of casting to u32 this should be try_into() with conversion into Error, clearly stating that we can't represent dates past 2038.

Byron · 2022-08-24T01:55:55Z

git-date/tests/time/parse.rs

+
+#[test]
+fn relative() {
+    let two_weeks_ago = git_date::parse("2 weeks ago").expect("valid time");


I am aware that this implementation is not complete, but think every possible input that you know should be working should also be tested here. I can imagine having a loop like for (input, expect) in [(…), (…)] {} to do that conveniently.

Byron

(Some more notes related to making parse a pure function)

Byron · 2022-08-24T02:14:40Z

git-date/src/parse.rs

 use crate::Time;
+use time::{Date, OffsetDateTime};

 #[allow(missing_docs)]
 pub fn parse(input: &str) -> Option<Time> {


Before I forget, the current time should be passed here. That way tests are deterministic.

Byron · 2022-08-24T02:16:09Z

git-date/tests/time/parse.rs

+
+#[test]
+fn relative() {
+    let two_weeks_ago = git_date::parse("2 weeks ago").expect("valid time");


These tests will be easier to write once parse(…) takes the current time and is 'pure'.

specific test names

svetli-n · 2022-08-26T21:18:29Z

Regrading #498 (comment) I think its local offset.

Byron · 2022-08-27T08:33:45Z

Session Notes

Ways to dig deeper into how date parsing is used:

baseline includes actual timestamp created by git
gix rev parse '@{1979-02-26 18:30:00}' -e - should use a nicer format when displaying the parsed time
The above without -e shows how the date could be used (even though right now it isn't) to search the reflog

Byron

Thanks for the updates, I added some notes as well.

Byron · 2022-08-30T01:51:51Z

git-date/tests/time/parse.rs

+        );
+        if *exit_code == 0 {
+            let actual = res.unwrap().seconds_since_unix_epoch;
+            let expected = u32::from_str(output.to_str().expect("valid utf")).expect("valid epoch value");


I think it's better to parse the output while parsing the baseline file, that way it can be used natively here.

Byron · 2022-08-30T01:53:38Z

git-date/src/parse.rs

    // TODO: actual implementation, this is just to not constantly fail
    if input == "1979-02-26 18:30:00" {
-        Some(Time::new(42, 1800))
+        Ok(Time::new(42, 1800))
    } else {
        return if let Ok(val) = Date::parse(input, SHORT) {


Explicit return isn't needed here.
Also you can wrap the whole if expression expression into an Ok to save on all the Ok in each branch.

Changed to not explicit return, thanks but don't understand "wrap the whole if expression", example may be?

Byron · 2022-08-30T01:57:02Z

git-revision/src/spec/parse/function.rs

@@ -435,7 +435,7 @@ where
                let time = nav
                    .to_str()
                    .ok()
-                    .and_then(git_date::parse)
+                    .and_then(|v| git_date::parse(v).ok())


Instead of degenerating the error it should be favorable to put it into .ok_or_else(|| Error::Time { input: nav.into() , source: err})?; like so.

Not sure if I understand, and_then needs to return Option and git_date::parse returns Result

Byron · 2022-08-30T01:59:31Z

git-date/src/parse.rs

@@ -55,8 +58,11 @@ pub fn parse(input: &str) -> Result<Time, Error> {
        } else if let Some(val) = parse_raw(input) {
            // Format::Raw
            Ok(val)
-        } else if let Some(val) = relative::parse(input) {
-            Ok(Time::new(val.unix_timestamp() as u32, val.offset().whole_seconds()))
+        } else if let Some(val) = relative::parse(input, now.ok_or(Error::MissingCurrentTime)?) {


This would cause unnecessary errors if no time is given but it's not a relative date either. Instead, let the relative::parse() function return a time offset (it's relative, after all), and if it succeeded and there is no current time, we have to fail. There should be a test for that, too.

This would cause unnecessary errors if no time is given but it's not a relative date either.

See 1eac4de#diff-fe1f4921b662e3ab738c66e74ec7da02aa5ffde9cc94fe9c1a17e90326f54813R74 no error if passing None

Byron · 2022-09-01T00:00:48Z

Thank you! I hope it's OK to handle the questions during our session, I am so swamped right now.

Byron · 2022-09-03T09:33:24Z

I couldn't fix that git-lfs issue, too strange, so I am merging what we have and kindly ask you to move the checkboxes into a new PR :D.

Thank you

svetli-n · 2022-09-05T19:56:45Z

I couldn't fix that git-lfs issue, too strange, so I am merging what we have and kindly ask you to move the checkboxes into a new PR :D.

Thank you

Done, created #527 and #528

This extends `gix_date::parse::relative::span` to recognize times with "months" and "years", as in "3 months ago" and "2 years ago". The actual change here is very simple, because it is directly facilitated by GitoxideLabs#1474. The units "seconds", "minutes", "hours", "days", and "weeks" continue to be recognized (as before). Relative times like "1 year, 2 months ago" remain unrecognized. For background, see 43b6c06 (GitoxideLabs#498), c5c6bf6, GitoxideLabs#1474 (comment), and GitoxideLabs#1696 (comment). Note that this change does not fix issue GitoxideLabs#1696, which continues to apply to the intepretation of "days" and "weeks", and now also applies in the same way to the interpretation of "months" and "years". (It continues not to apply to the interpretation of "seconds", "minutes", and "hours".) The tests are updated to cover "months" and "years", as well as to exercise a wider range of relative times, including showing which units (e.g. "days") are affected by GitoxideLabs#1696. A few sub-cases, of those adjusted or added here, test strings from a related test in Git, to allow comparison. But most are not related to cases there. As before, the tests pass on CI, or when the time zone is otherwise UTC, or with local time zones that never have DST adjustments. The sub-cases are reordered to be monotone increasing in the magnitude of the relative intervals tested (and thus monotone decreasing in the associated absolute timestamps), and the original input is kept zipped into the `actual` and `expected` arrays being compared. This way, if the assertion fails (such as due to GitoxideLabs#1696), it is clear and readable which sub-cases failed and exactly how, as well as what all the sub-cases are and what each sub-case tests. Reordering the sub-cases and preserving the inputs they parse in this way avoids the disadvantages of GitoxideLabs#1697 while keeping, and expanding on, its benefits. Failure, where it occurs, now looks like: --- STDERR: gix-date::date time::parse::relative::various --- thread 'time::parse::relative::various' panicked at gix-date\tests\time\parse.rs:252:9: assertion failed: `(left == right)`: relative times differ Diff < left / right > : [ ( "5 seconds ago", 2024-11-24T23:51:49Z, ), ( "5 minutes ago", 2024-11-24T23:46:54Z, ), ( "5 hours ago", 2024-11-24T18:51:54Z, ), ( "5 days ago", 2024-11-19T23:51:54Z, ), ( "3 weeks ago", 2024-11-03T23:51:54Z, ), ( "21 days ago", 2024-11-03T23:51:54Z, ), ( "504 hours ago", 2024-11-03T23:51:54Z, ), ( "30240 minutes ago", 2024-11-03T23:51:54Z, ), ( "2 months ago", < 2024-09-24T23:51:54Z, > 2024-09-24T22:51:54Z, ), ( "1460 hours ago", 2024-09-25T03:51:54Z, ), ( "87600 minutes ago", 2024-09-25T03:51:54Z, ), ( "14 weeks ago", < 2024-08-18T23:51:54Z, > 2024-08-18T22:51:54Z, ), ( "98 days ago", < 2024-08-18T23:51:54Z, > 2024-08-18T22:51:54Z, ), ( "2352 hours ago", 2024-08-18T23:51:54Z, ), ( "141120 minutes ago", 2024-08-18T23:51:54Z, ), ( "5 months ago", < 2024-06-24T23:51:54Z, > 2024-06-24T22:51:54Z, ), ( "3650 hours ago", 2024-06-25T21:51:54Z, ), ( "219000 minutes ago", 2024-06-25T21:51:54Z, ), ( "26 weeks ago", < 2024-05-26T23:51:54Z, > 2024-05-26T22:51:54Z, ), ( "182 days ago", < 2024-05-26T23:51:54Z, > 2024-05-26T22:51:54Z, ), ( "4368 hours ago", 2024-05-26T23:51:54Z, ), ( "262080 minutes ago", 2024-05-26T23:51:54Z, ), ( "8 months ago", < 2024-03-24T23:51:54Z, > 2024-03-24T22:51:54Z, ), ( "5840 hours ago", 2024-03-26T15:51:54Z, ), ( "350400 minutes ago", 2024-03-26T15:51:54Z, ), ( "38 weeks ago", 2024-03-03T23:51:54Z, ), ( "266 days ago", 2024-03-03T23:51:54Z, ), ( "6384 hours ago", 2024-03-03T23:51:54Z, ), ( "383040 minutes ago", 2024-03-03T23:51:54Z, ), ( "11 months ago", 2023-12-24T23:51:54Z, ), ( "8030 hours ago", 2023-12-26T09:51:54Z, ), ( "481800 minutes ago", 2023-12-26T09:51:54Z, ), ( "14 months ago", < 2023-09-24T23:51:54Z, > 2023-09-24T22:51:54Z, ), ( "21 months ago", 2023-02-24T23:51:54Z, ), ( "2 years ago", 2022-11-24T23:51:54Z, ), ( "20 years ago", 2004-11-24T23:51:54Z, ), ( "630720000 seconds ago", 2004-11-29T23:51:54Z, ), ]

This extends `gix_date::parse::relative::span` to recognize times with "months" and "years", as in "3 months ago" and "2 years ago". Those units are supported by Git. The actual change here to the parsing code is very simple, because it is directly facilitated by GitoxideLabs#1474. The units "seconds", "minutes", "hours", "days", and "weeks" continue to be recognized (as before). Relative times like "1 year, 2 months ago" remain unrecognized. For background, see 43b6c06 (GitoxideLabs#498), c5c6bf6, GitoxideLabs#1474 (comment), and GitoxideLabs#1696 (comment). Note that this specific change does not itself fix issue GitoxideLabs#1696, which applies to the intepretation of "days" and "weeks", and now also applies in the same way to the interpretation of "months" and "years". (It continues not to apply to the interpretation of "seconds", "minutes", and "hours".) The tests are updated to cover "months" and "years", as well as to exercise a wider range of relative times, including showing which units (e.g. "days") are affected by GitoxideLabs#1696. A few sub-cases, of those adjusted or added here, test strings from a related test in Git, to allow comparison. But most are not related to cases there. As before, the tests pass on CI, or when the time zone is otherwise UTC, or with local time zones that never have DST adjustments. The sub-cases are reordered to be monotone increasing in the magnitude of the relative intervals tested (and thus monotone decreasing in the associated absolute timestamps), and the original input is kept zipped into the `actual` and `expected` arrays being compared. This way, if the assertion fails (such as due to GitoxideLabs#1696), it is clear and readable which sub-cases failed and exactly how, as well as what all the sub-cases are and what each sub-case tests. Reordering the sub-cases and preserving the inputs they parse in this way avoids the disadvantages of GitoxideLabs#1697 while keeping, and expanding on, its benefits. Failure, where it occurs, now looks like: --- STDERR: gix-date::date time::parse::relative::various --- thread 'time::parse::relative::various' panicked at gix-date\tests\time\parse.rs:252:9: assertion failed: `(left == right)`: relative times differ Diff < left / right > : [ ( "5 seconds ago", 2024-11-24T23:51:49Z, ), ( "5 minutes ago", 2024-11-24T23:46:54Z, ), ( "5 hours ago", 2024-11-24T18:51:54Z, ), ( "5 days ago", 2024-11-19T23:51:54Z, ), ( "3 weeks ago", 2024-11-03T23:51:54Z, ), ( "21 days ago", 2024-11-03T23:51:54Z, ), ( "504 hours ago", 2024-11-03T23:51:54Z, ), ( "30240 minutes ago", 2024-11-03T23:51:54Z, ), ( "2 months ago", < 2024-09-24T23:51:54Z, > 2024-09-24T22:51:54Z, ), ( "1460 hours ago", 2024-09-25T03:51:54Z, ), ( "87600 minutes ago", 2024-09-25T03:51:54Z, ), ( "14 weeks ago", < 2024-08-18T23:51:54Z, > 2024-08-18T22:51:54Z, ), ( "98 days ago", < 2024-08-18T23:51:54Z, > 2024-08-18T22:51:54Z, ), ( "2352 hours ago", 2024-08-18T23:51:54Z, ), ( "141120 minutes ago", 2024-08-18T23:51:54Z, ), ( "5 months ago", < 2024-06-24T23:51:54Z, > 2024-06-24T22:51:54Z, ), ( "3650 hours ago", 2024-06-25T21:51:54Z, ), ( "219000 minutes ago", 2024-06-25T21:51:54Z, ), ( "26 weeks ago", < 2024-05-26T23:51:54Z, > 2024-05-26T22:51:54Z, ), ( "182 days ago", < 2024-05-26T23:51:54Z, > 2024-05-26T22:51:54Z, ), ( "4368 hours ago", 2024-05-26T23:51:54Z, ), ( "262080 minutes ago", 2024-05-26T23:51:54Z, ), ( "8 months ago", < 2024-03-24T23:51:54Z, > 2024-03-24T22:51:54Z, ), ( "5840 hours ago", 2024-03-26T15:51:54Z, ), ( "350400 minutes ago", 2024-03-26T15:51:54Z, ), ( "38 weeks ago", 2024-03-03T23:51:54Z, ), ( "266 days ago", 2024-03-03T23:51:54Z, ), ( "6384 hours ago", 2024-03-03T23:51:54Z, ), ( "383040 minutes ago", 2024-03-03T23:51:54Z, ), ( "11 months ago", 2023-12-24T23:51:54Z, ), ( "8030 hours ago", 2023-12-26T09:51:54Z, ), ( "481800 minutes ago", 2023-12-26T09:51:54Z, ), ( "14 months ago", < 2023-09-24T23:51:54Z, > 2023-09-24T22:51:54Z, ), ( "21 months ago", 2023-02-24T23:51:54Z, ), ( "2 years ago", 2022-11-24T23:51:54Z, ), ( "20 years ago", 2004-11-24T23:51:54Z, ), ( "630720000 seconds ago", 2004-11-29T23:51:54Z, ), ]

svetli-n changed the title ~~Draft.~~ git-date parse draft Aug 23, 2022

svetli-n marked this pull request as draft August 23, 2022 20:56

svetli-n commented Aug 23, 2022

View reviewed changes

git-date/src/parse.rs Show resolved Hide resolved

Byron reviewed Aug 24, 2022

View reviewed changes

svetli-n and others added 5 commits August 26, 2022 22:46

Draft.

43b6c06

Draft.

95b4902

refactor

3e6e0f9

specific test names

Add git baseline.

b747a60

Add fixtures.

6c40ac1

svetli-n force-pushed the git_date_parse branch from 82e3242 to 6c40ac1 Compare August 26, 2022 20:46

parse returns Result.

67c8c6a

svetli-n added 4 commits August 29, 2022 21:18

Add output to baseline.

5c3b733

parse() returns Result.

206f392

Fallible timestamp cast i64 -> u32.

cce7616

parse is pure function.

9ad1a5f

Byron reviewed Aug 30, 2022

View reviewed changes

PR comments.

1eac4de

Byron merged commit 1eac4de into GitoxideLabs:main Sep 3, 2022

EliahKagan mentioned this pull request Nov 25, 2024

Parse relative months and years #1702

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

git-date parse draft #498

git-date parse draft #498

svetli-n commented Aug 23, 2022 •

edited

Loading

svetli-n commented Aug 23, 2022

Byron left a comment •

edited

Loading

Byron Aug 24, 2022

Byron Aug 24, 2022

Byron Aug 24, 2022

Byron left a comment

Byron Aug 24, 2022

Byron Aug 24, 2022

svetli-n commented Aug 26, 2022

Byron commented Aug 27, 2022 •

edited

Loading

Byron left a comment

Byron Aug 30, 2022

Byron Aug 30, 2022

svetli-n Aug 31, 2022

Byron Aug 30, 2022

svetli-n Aug 31, 2022

Byron Aug 30, 2022

svetli-n Aug 31, 2022

Byron commented Sep 1, 2022

Byron commented Sep 3, 2022

svetli-n commented Sep 5, 2022

git-date parse draft #498

git-date parse draft #498

Conversation

svetli-n commented Aug 23, 2022 • edited Loading

Tasks

svetli-n commented Aug 23, 2022

Byron left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Byron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svetli-n commented Aug 26, 2022

Byron commented Aug 27, 2022 • edited Loading

Session Notes

Byron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Byron commented Sep 1, 2022

Byron commented Sep 3, 2022

svetli-n commented Sep 5, 2022

svetli-n commented Aug 23, 2022 •

edited

Loading

Byron left a comment •

edited

Loading

Byron commented Aug 27, 2022 •

edited

Loading