uutils / parse_datetime

Parses a relative time string and returns a `Duration`
MIT License
14 stars 17 forks source link

Support weekdays in parse_datetime #34

Closed philolo1 closed 9 months ago

philolo1 commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

So should i wait for https://github.com/uutils/parse_datetime/pull/25 to be merged and then refactor?

tertsdiepraam commented 10 months ago

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:

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 commented 10 months ago

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:

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 commented 10 months ago

Sounds good to me!

philolo1 commented 10 months ago

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

philolo1 commented 10 months ago

I converted it to draft, sorry.

tertsdiepraam commented 10 months ago

No worries! Looking forward to the nom version!

philolo1 commented 10 months ago

@tertsdiepraam it should be ready to review now.

philolo1 commented 10 months ago

@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 commented 10 months ago

@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 commented 10 months ago

@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 commented 10 months ago

Here is the issue: #39

philolo1 commented 10 months ago

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

philolo1 commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

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

sylvestre commented 9 months ago

thanks