Support weekdays in parse_datetime #34

philolo1 · 2023-08-23T01:53:31Z

@tertsdiepraam @cakebaker @sylvestre
This commit resolves issue #23.

Adds parse_weekday function that uses chrono weekday parser with a map for edge cases. Adds tests cases to make sure it works correctly.

tertsdiepraam · 2023-08-23T07:23:55Z

Just to note, we (at least I) already see updates for all uutils crates when I log on to github. Mentions don't really help, we'll look at the PR without them too 🙂

philolo1 · 2023-08-24T11:43:07Z

So should i wait for #25 to be merged and then refactor?

tertsdiepraam · 2023-08-24T14:16:35Z

Hmm yeah I'm not really sure what the best course of action is. The situation is a bit unfortunate. I guess it depends on a few things:

Will Make a beginning of parsing GNU date items with nom #25 be ready soon?
How relevant is this PR to Make a beginning of parsing GNU date items with nom #25, i.e. can we reuse significant parts of this with nom?

The first question is kinda tough. I think a basic nom version of this library should be merged soon, just to get the transition going. Which might not be the entirety of #25. The second question is interesting. At the least the tests are relevant after the introduction of nom.

Maybe another approach is to transition to nom from the bottom up. What I mean with that is that we port small parts first and gradually build up to parsing the whole thing with nom. For example, if we make a nom-based` function to parse weekdays, we can easily integrate that later.

I'm really just explaining my thoughts here. I'm not yet really sure what the best approach is. What do you think?

philolo1 · 2023-08-24T14:50:20Z

Hmm yeah I'm not really sure what the best course of action is. The situation is a bit unfortunate. I guess it depends on a few things:

Will Make a beginning of parsing GNU date items with nom #25 be ready soon?

How relevant is this PR to Make a beginning of parsing GNU date items with nom #25, i.e. can we reuse significant parts of this with nom?

The first question is kinda tough. I think a basic nom version of this library should be merged soon, just to get the transition going. Which might not be the entirety of #25. The second question is interesting. At the least the tests are relevant after the introduction of nom.

Maybe another approach is to transition to nom from the bottom up. What I mean with that is that we port small parts first and gradually build up to parsing the whole thing with nom. For example, if we make a nom-based` function to parse weekdays, we can easily integrate that later.

I'm really just explaining my thoughts here. I'm not yet really sure what the best approach is. What do you think?

How about waiting until next week and see the progress of #25. If nothing happens i can rewrite this pr first to use nom and check the pr.

tertsdiepraam · 2023-08-24T15:09:30Z

Sounds good to me!

src/parse_weekday.rs

src/lib.rs

.gitignore

philolo1 · 2023-08-29T13:11:46Z

@tertsdiepraam thanks for reviewing, but i am currently rewriting it to use nom.

philolo1 · 2023-08-29T13:13:08Z

I converted it to draft, sorry.

tertsdiepraam · 2023-08-29T13:15:13Z

No worries! Looking forward to the nom version!

philolo1 · 2023-08-29T14:22:39Z

@tertsdiepraam it should be ready to review now.

tertsdiepraam

Interesting! I quite like how that's looking. It's definitely a nice way to start using nom!

src/parse_weekday.rs

src/lib.rs

src/parse_weekday.rs

philolo1 · 2023-08-30T02:09:48Z

@tertsdiepraam i did all the requests accept one, i am not sure what "m" means, but it does not seem to be monday.

I just tested all letters from a to z with date and this is the result:

Current date:
Wed Aug 30 11:10:08 JST 2023
a
Wed Aug 30 08:00:00 JST 2023
b
Wed Aug 30 07:00:00 JST 2023
c
Wed Aug 30 06:00:00 JST 2023
d
Wed Aug 30 05:00:00 JST 2023
e
Wed Aug 30 04:00:00 JST 2023
f
Wed Aug 30 03:00:00 JST 2023
g
Wed Aug 30 02:00:00 JST 2023
h
Wed Aug 30 01:00:00 JST 2023
i
Wed Aug 30 00:00:00 JST 2023
j
date: invalid date ‘j’
k
Tue Aug 29 23:00:00 JST 2023
l
Tue Aug 29 22:00:00 JST 2023
m
Tue Aug 29 21:00:00 JST 2023
n
Wed Aug 30 10:00:00 JST 2023
o
Wed Aug 30 11:00:00 JST 2023
p
Wed Aug 30 12:00:00 JST 2023
q
Wed Aug 30 13:00:00 JST 2023
r
Wed Aug 30 14:00:00 JST 2023
s
Wed Aug 30 15:00:00 JST 2023
t
Wed Aug 30 16:00:00 JST 2023
u
Wed Aug 30 17:00:00 JST 2023
v
Wed Aug 30 18:00:00 JST 2023
w
Wed Aug 30 19:00:00 JST 2023
x
Wed Aug 30 20:00:00 JST 2023
y
Wed Aug 30 21:00:00 JST 2023
z
Wed Aug 30 09:00:00 JST 2023

philolo1 · 2023-08-30T02:21:22Z

@tertsdiepraam i believe i found the reference source code, apparently "WEDNES" is also valid. https://github.com/coreutils/gnulib/blob/master/lib/parse-datetime.y#L1032

philolo1 · 2023-08-30T02:30:50Z

@tertsdiepraam apparently one letter is military time table, i will create another issue.

https://github.com/coreutils/gnulib/blob/master/lib/parse-datetime.y#L1155

philolo1 · 2023-08-30T02:37:01Z

Here is the issue: #39

tertsdiepraam

Nice work! I have just two more comments for things that should not be fixed in this PR and one question.

It's great that parse_datetime is receiving some proper attention from you. Thanks!

tertsdiepraam · 2023-08-30T06:12:50Z

src/lib.rs

@@ -168,6 +173,27 @@ pub fn parse_datetime_at_date<S: AsRef<str> + Clone>(
        }
    }

+    // parse weekday
+    if let Some(weekday) = parse_weekday::parse_weekday(s.as_ref()) {
+        let mut beginning_of_day = date


Oh I see what the trouble was now. I forgot we needed 00:00 on the time. I'm not sure what the best way to do that is. This looks ok, but it would be great if chrono had some nice method for this, but I can't find any 😄

i dit not find anything either :(

tertsdiepraam · 2023-08-30T06:14:20Z

src/lib.rs

+            beginning_of_day += Duration::days(1);
+        }
+
+        if let Ok(dt) = naive_dt_to_fixed_offset(date, beginning_of_day.naive_local()) {


Does using From<DateTime<FixedOffset>> for DateTime<Local> work here or is the naive_local necessary?

@tertsdiepraam seems it works, thanks.

tertsdiepraam · 2023-08-30T06:16:52Z

src/lib.rs

+        #[test]
+        fn test_weekday() {
+            // add some constant hours and minutes and seconds to check its reset
+            let date = Local.with_ymd_and_hms(2023, 02, 28, 10, 12, 3).unwrap();


Oww, I really want parse_datetime_at_date to not take Local anymore. But we'll get to that in another PR. Looks great for now!

I am not sure how to best not use local but agreed.

Yeah, parse_datetime_at_date should just be generic over the timezone. And then it can be called with Utc, Local or FixedOffset. Like I said though, definitely out of scope for this PR.

philolo1 · 2023-08-30T06:49:55Z

@tertsdiepraam Thank for the review. I am trying to improve my rust coding skills and contributing to open source is a good opportunity.

philolo1 · 2023-08-31T08:59:30Z

@tertsdiepraam I am not to familiar with github, it still shows changed requested. Do i have to do something to request review. Furthermore, should i squash the commits or would you do that when you merge it?

tertsdiepraam · 2023-08-31T14:39:28Z

The "changes requested" will stay there until I approve, but it's not enforced so no need to worry about it. I've decided to not write/review any uutils code the next few weeks to stip myself from using it to procrastinate on an important project. The review will probably soon be continued by another maintainer when they have time. If you need me specifically you can still mention me. :)

Also squashing would be nice! We could do it, but we might forget, so might be best if you do it.

philolo1 · 2023-09-01T05:00:30Z

I squashed, would be nice if someone can get this merged.

src/parse_weekday.rs

sylvestre · 2023-09-01T07:55:06Z

src/parse_weekday.rs

+
+    match parse_result {
+        Ok((_, weekday)) => Some(weekday),
+        Err(_) => None,


why don't you return a Result ?
here, it would be
Err(_) => Err("Failed to parse weekday"),

I had this discussion with @tertsdiepraam, but I think the reason is this is used in parse_datetime and we only care about whether it was parsed or not.

My reasoning was that we only care about the parsing the whole datetime. If a weekday fails, another type of item might still succeedsl.

@sylvestre should i change it to Result?

@tertsdiepraam @sylvestre its been a while, i am fine with whatever but it would be good to get this pr also merged, before we forget what it was about :) .

sylvestre

sorry, i didn't submitted my comments :(

sylvestre · 2023-09-01T08:07:40Z

src/parse_weekday.rs

+    let s = s.as_str();
+
+    let parse_result: IResult<&str, Weekday> = nom::combinator::all_consuming(alt((
+        value(Weekday::Mon, alt((tag("monday"), tag("mon")))),


what about using a macro like

// Helper macro to simplify tag matching macro_rules! tag_match { ($day:expr, $($pattern:expr),+) => { value($day, alt(($(tag($pattern)),+))) }; }

to have

tag_match!(Weekday::Mon, "monday", "mon"),

@sylvestre done

src/parse_weekday.rs

This commit resolves issue uutils#23. Adds parse_weekday function that uses chrono weekday parser with a map for edge cases. Adds tests cases to make sure it works correctly. Use nom for parsing.

sylvestre · 2023-09-19T09:44:19Z

thanks

philolo1 force-pushed the fix/23-weekday branch 3 times, most recently from 55badc9 to de2ef37 Compare August 29, 2023 12:45

tertsdiepraam reviewed Aug 29, 2023

View reviewed changes

src/parse_weekday.rs Outdated Show resolved Hide resolved

tertsdiepraam reviewed Aug 29, 2023

View reviewed changes

src/parse_weekday.rs Outdated Show resolved Hide resolved

tertsdiepraam reviewed Aug 29, 2023

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

.gitignore Outdated Show resolved Hide resolved

philolo1 marked this pull request as draft August 29, 2023 13:12

philolo1 force-pushed the fix/23-weekday branch 2 times, most recently from 2cfe2fd to fe29267 Compare August 29, 2023 14:21

philolo1 marked this pull request as ready for review August 29, 2023 14:22

philolo1 force-pushed the fix/23-weekday branch from fe29267 to f072913 Compare August 29, 2023 14:22

tertsdiepraam requested changes Aug 29, 2023

View reviewed changes

philolo1 force-pushed the fix/23-weekday branch from 0c86935 to 1ab0e0d Compare August 30, 2023 00:21

philolo1 marked this pull request as draft August 30, 2023 00:23

philolo1 force-pushed the fix/23-weekday branch 2 times, most recently from a4b5423 to 46bfd1c Compare August 30, 2023 01:35

philolo1 marked this pull request as ready for review August 30, 2023 01:35

philolo1 force-pushed the fix/23-weekday branch from 46bfd1c to 82b2294 Compare August 30, 2023 02:23

philolo1 force-pushed the fix/23-weekday branch from 82b2294 to 93be2f5 Compare August 30, 2023 03:41

tertsdiepraam reviewed Aug 30, 2023

View reviewed changes

philolo1 force-pushed the fix/23-weekday branch from 93be2f5 to ea4b709 Compare August 30, 2023 06:46

philolo1 requested a review from tertsdiepraam August 30, 2023 06:51

philolo1 force-pushed the fix/23-weekday branch from ea4b709 to 83189e0 Compare September 1, 2023 05:01

sylvestre reviewed Sep 1, 2023

View reviewed changes

src/parse_weekday.rs Show resolved Hide resolved

sylvestre reviewed Sep 1, 2023

View reviewed changes

philolo1 force-pushed the fix/23-weekday branch from 83189e0 to 8746c65 Compare September 1, 2023 07:59

sylvestre reviewed Sep 15, 2023

View reviewed changes

Support weekdays in parse_datetime

96db1a2

This commit resolves issue uutils#23. Adds parse_weekday function that uses chrono weekday parser with a map for edge cases. Adds tests cases to make sure it works correctly. Use nom for parsing.

philolo1 force-pushed the fix/23-weekday branch from 8746c65 to 96db1a2 Compare September 19, 2023 08:47

philolo1 requested a review from sylvestre September 19, 2023 08:50

sylvestre merged commit 2d05b14 into uutils:main Sep 19, 2023
14 checks passed

Support weekdays in parse_datetime #34

Support weekdays in parse_datetime #34

Conversation

philolo1 commented Aug 23, 2023

tertsdiepraam commented Aug 23, 2023

philolo1 commented Aug 24, 2023

tertsdiepraam commented Aug 24, 2023

philolo1 commented Aug 24, 2023

tertsdiepraam commented Aug 24, 2023

philolo1 commented Aug 29, 2023

philolo1 commented Aug 29, 2023

tertsdiepraam commented Aug 29, 2023

philolo1 commented Aug 29, 2023

tertsdiepraam left a comment

Choose a reason for hiding this comment

philolo1 commented Aug 30, 2023 • edited Loading

philolo1 commented Aug 30, 2023

philolo1 commented Aug 30, 2023

philolo1 commented Aug 30, 2023

tertsdiepraam 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

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

philolo1 commented Aug 30, 2023

philolo1 commented Aug 31, 2023

tertsdiepraam commented Aug 31, 2023

philolo1 commented Sep 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philolo1 Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sylvestre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sylvestre commented Sep 19, 2023

philolo1 commented Aug 30, 2023 •

edited

Loading

tertsdiepraam left a comment •

edited

Loading

philolo1 Sep 4, 2023 •

edited

Loading