PicoJr / htp

human time parser lib
Apache License 2.0
6 stars 4 forks source link

Add future_if_past #3

Closed Geobert closed 3 years ago

Geobert commented 3 years ago

If a time gives a date < now, and future_if_past is true, we bump the date to the next day Wrapped the flag into a Cfg object if ever we expand the parameters in the future

PicoJr commented 3 years ago

Thanks for your contribution.

Feature

This feature makes sense, implementation and test looks good.

Cfg and API

I have mixed feeling regarding the Cfg struct: I doubt the parameters will need to be extended and it looks cumbersome.

I would rather keep now as a standalone parameter for now instead of burying it under a struct:

I prefer:

pub fn evaluate<Tz: chrono::TimeZone>(
    time_clue: TimeClue,
    now: DateTime<Tz>,
    /// assume next day if only time is supplied and time < now
    assume_next_day: bool,
) -> Result<DateTime<Tz>, EvaluationError> {...}

over

pub fn evaluate<Tz: chrono::TimeZone>(
    time_clue: TimeClue,
    cfg: &Cfg<Tz>,
) -> Result<DateTime<Tz>, EvaluationError> {...}

Note: evaluate is a pub fn so modifying its signature is a breaking API change, for this reason I would rather rename it to keep backward compatibility:

/// same as evaluate_time_clue(time_clue, now, false)
/// ie assume times are relative to current day, even if time is past
pub fn evaluate<Tz: chrono::TimeZone>(
    time_clue: TimeClue,
    now: DateTime<Tz>,
) -> Result<DateTime<Tz>, EvaluationError> {evaluate_time_clue(time_clue, now, false)}

pub fn evaluate_time_clue<Tz: chrono::TimeZone>(
    time_clue: TimeClue,
    now: DateTime<Tz>,
    /// assume next day if only time is supplied and time < now
    assume_next_day: bool, 
) -> Result<DateTime<Tz>, EvaluationError> {...}

I will be happy to merge your PR with these changes if you are okay with them.

Thanks again.

Geobert commented 3 years ago

Thanks for reviewing this! Sorry for missing out the pub on evaluate, I've was cautious on parse but oversight evaluate.

I'll modify the PR as you feel it after work. I wrapped the params into a struct because of two_timer, another crate that parses human time, but it lacks of relative time (in 10 min) and the fact that the code wasn't as obvious as yours made me switch. And htp is way smaller too :)

This crate uses a Config struct for its parameters as it has some of them, that's where my inspiration came from :)

Geobert commented 3 years ago

It is done! :)