Make parse_datetime::parse_datetime::from_str accept relative time

[philolo1]

@tertsdiepraam @sylvestre

As discussed in https://github.com/uutils/coreutils/pull/5181, this makes parse_dateime::parse_datetime::from_str accept relative time.

[tertsdiepraam]

I think this is an opportunity to clean up the interface. There's no reason to expose multiple functions with overlapping functionality. The only public items should be:

• A parse_datetime function (module should be private and be renamed)
• A parse_datetime_at_date function
• ParseDateTimeError (renamed from ParseDurationError)

The parse_datetime module could be called datetime_format maybe (or some other name).

This would hide functions that we are currently using in coreutils, but those should be forced to change :)

[philolo1]

That makes sense. I think i can tackle that, but with those changes we should probably also increase the version number. It might cause quite some conflict for existing prs.

On Mon, Aug 21, 2023 at 21:01 Terts Diepraam @.***> wrote:

I think this is an opportunity to clean up the interface. There's no reason to expose multiple functions with overlapping functionality. The only public items should be:

• A parse_datetime function (module should be private and be renamed)
• A parse_datetime_at_date function
• ParseDateTimeError (renamed from ParseDurationError)

The parse_datetime module could be called datetime_format maybe (or some other name).

This would hide functions that we are currently using in coreutils, but those should be forced to change :)

— Reply to this email directly, view it on GitHub https://github.com/uutils/parse_datetime/pull/33#issuecomment-1686198822, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABP4LZTFKHNL6UZSISZYKBDXWNEZTANCNFSM6AAAAAA3YHMPWE . You are receiving this because you authored the thread.Message ID: @.***>

[tertsdiepraam]

That makes sense. I think i can tackle that

Great! Thanks!

but with those changes we should probably also increase the version number.

I agree, we'll bump the version number on the next release.

It might cause quite some conflict for existing prs.

Indeed. It's a bit unfortunate, but the sooner we do this the better in my opinion.

[philolo1]

@tertsdiepraam I have done some refactoring. When you have some time, can you take a look a look (its quite a big renaming / refactor, so i am sure there will be some revisions).

Here is what i did:

• put the parse_datetime into lib.rs
• Create new file parse_relative_time.rs with the relative time helper function.
• Renames from_str to parse_datetime and parse_relative_time.
• Adds function parse_datetime_at_date
• Update Readme.
• Update version number as interfaces changes

[tertsdiepraam]

We'll get to the version number once we actually publish. You can revert that for now.

Also, edition in Cargo.toml refers to the Rust edition. There's no 2023 edition. 2021 is the latest (and the next one will be 2024). That's what makes the CI fail.

[philolo1]

@tertsdiepraam Sure, and thanks for the comments. I will mention you ones the pipeline is fixed.

[philolo1]

@tertsdiepraam the pipeline is passing now.

[sylvestre]

needs to be rebased :)

[philolo1]

@sylvestre rebased

[philolo1]

Any updates/ feedback?

[tertsdiepraam]

It's only been a day. We're not that fast ;p

I'll take a look soon!

[philolo1]

@tertsdiepraam thanks for taking the time to review this.

[philolo1]

@tertsdiepraam i believe i have adjusted the code according to your review. As its a big change of the library, take your time to review it.

[philolo1]

@tertsdiepraam i believe i fixed all issues.

[sylvestre]

@philolo1 is it by design that parse_relative_time is private? I am trying to use it in the rust coreutils:

error[E0603]: function `parse_relative_time` is private
  --> src/uu/touch/src/touch.rs:92:49
   |
92 |             if let Ok(offset) = parse_datetime::parse_relative_time(date) {
   |                                                 ^^^^^^^^^^^^^^^^^^^ private function

[philolo1]

@sylvestre i am sorry I do not remember why we only exposed one function. I dont see any issue also exposing this function. Do you remember the reason @tertsdiepraam ? @sylvestre it would be fine with me to expose this.

[sylvestre]

@philolo1 yeah, it is significantly reducing the scope of this crate :)

could you please make the change? thanks

[philolo1]

Sure i can do it either today or tomorrow @sylvestre , then we should create a new version

[sylvestre]

actually, i did it here :) https://github.com/uutils/parse_datetime/pull/50

[philolo1]

Thanks

On Sat, Sep 23, 2023 at 17:41 Sylvestre Ledru @.***> wrote:

actually, i did it here :)

50 https://github.com/uutils/parse_datetime/pull/50

— Reply to this email directly, view it on GitHub https://github.com/uutils/parse_datetime/pull/33#issuecomment-1732256148, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABP4LZTN6DPSMFCAIRAQVKLX32OB7ANCNFSM6AAAAAA3YHMPWE . You are receiving this because you were mentioned.Message ID: @.***>

[tertsdiepraam]

Do you remember the reason @tertsdiepraam ?

Yes, there's a good reason not to do relative time only, because a relative time on its own is undefined. For example, if we get a relative time of "1 month", how many days should we go forward? We've tried to work around this a bit, but that makes it more difficult. Instead we should always parse the full datetime relative to some reference datetime (which is the current time by default). So, in that format both "tomorrow" and "2023-09-23" are supported. I don't think there's anything in the coreutils that accept only a relative time, but not an absolute time as well. We haven't reduced the scope of the crate because the parse_datetime function is a superset of parse_relative_time in terms of functionality.

[sylvestre]

oh ok, my bad :)