Proper time support

[lyssieth]

• All tests pass, example works (as far as I can tell)
• Added chrono and time features, both enabled by default
• Added support for time crate

It's rather hacky, but it seems to work just fine.

[lyssieth]

Additionally, probably closes #5.

[lyssieth]

I'll add that the features are currently exclusive, however that could probably be solved.

[PicoJr]

Hello, thanks for your contribution. I have little time right now, but I will review your PR as soon as I can.

Btw: I tried to build using cargo build --features time, it fails because it tries to build htp with both chrono and time features.

cargo test --features time --no-default-features does work though.

I might add a compile time check to help future devs diagnose the issue.

Thanks again, I'll get back to you ASAP.

[lyssieth]

Yeah, I mentioned that they're currently mutually exclusive in my previous comment.

I can try and work on a less hacky version when I've got more time, I threw this together because I needed it (and use time in both front and backend)

[icepuma]

Hey folks,
I'm currently working on a similar lib, because htp development seemed to be dead. But this isn't the case obviously. My lib also tries to tackle multi-lang support for e.g. en = now, de = jetzt, fr = maintenant, ....

I got heavily inspired by your approach of using PEST files. The lib will offer more than just parsing human date strings, but I wonder if you want that multi-lang support as well?

Best regards
Stefan

P.S.: If you want, I can open an issue, combined with a PR 👍

[PicoJr]

Yeah, I mentioned that they're currently mutually exclusive in my previous comment.

I can try and work on a less hacky version when I've got more time, I threw this together because I needed it (and use time in both front and backend)

@lyssieth Thanks again for putting this together, I really appreciate.

It seems "features" should be additive if possible cf https://doc.rust-lang.org/cargo/reference/features.html?highlight=additive#feature-unification

I am wondering whether we could make sure that either or both chrono and time could be available at the same time.
It would allow both (or only one) API to coexist which would help downstream dependencies.

I guess the default could be default = ["chrono", "time"], maybe split time and chrono specific API in their own modules to avoid namespace collisions? What do you think?

[lyssieth]

Sounds good, yeah. I can separate the apis into their own modules, probably today or tomorrow depending on how time goes.

[lyssieth]

Oh, would it be okay if I fix all the lints as well? (ofc ensuring nothing breaks)

There's 30 warnings with cargo clippy (16 with check), and 45 if I add clippy::pedantic and clippy::nursery to a #![warn()]

[PicoJr]

Oh, would it be okay if I fix all the lints as well? (ofc ensuring nothing breaks)

There's 30 warnings with cargo clippy (16 with check), and 45 if I add clippy::pedantic and clippy::nursery to a #![warn()]

Yes, of course, thanks for helping me out. It used to be clean but clippy is now stricter 😅

[lyssieth]

Got a wee bit carried away, heh

[lyssieth]

It could probably use a good round of documentation

[PicoJr]

@lyssieth : FYI I am currently reading the new implementation.

I would like to make sure that all commands below compile the crate:

• cargo build --features chrono --no-default-features
• cargo build --features time --no-default-features
• cargo build --features chrono time

Unifying time and chrono API sounds good to me. I am wondering whether it is possible to define a trait in place of DateTime enum so that we can keep most of the code the same regardless of the backend. Each backend (chrono and time) would declare a New type and implement the unified trait.

IMO it is ok to provide one top level API for each backend (chrono and time), ie provide one parse and parse_time_clue per backend.

Both backend should be conditionally compiled based on selected features.

[lyssieth]

In theory a trait is possible, however might make passing values around a bit strange, might experiment a bit once free.

[lyssieth]

I also forgot to feature flag some things, sorry about that :>

[PicoJr]

I also forgot to feature flag some things, sorry about that :>

Thanks for your latest commit, it seems to work with both chrono, time and both at the same time.
It might need a bit of polishing but we are on the right track.

Maybe we should declare the unified enum as non exhaustive in case we need to support another backend someday?

I gave the "trait approach" a try and it is really not straightforward, I would rather stick to how you did it so far, it looks a lot more manageable ^^'

[lyssieth]

Maybe we should declare the unified enum as non exhaustive in case we need to support another backend someday?

Done!

It might need a bit of polishing but we are on the right track.

Agreed. There's documentation that needs doing (something I'm personally bad at), and probably some more testing?