nbari / cron-parser

cron parser
https://docs.rs/cron-parser/latest/cron_parser/
BSD 3-Clause "New" or "Revised" License
21 stars 3 forks source link

Support for specifying time zones #1

Closed jairinhohw closed 4 years ago

jairinhohw commented 5 years ago

I modified your crate for my use case, so it can use other timezones I don't know if it's a good implementation, but it seems to work

nbari commented 5 years ago

hi @jairinhohw many thanks for the fixes.

Instead of having to pass the timezone as a 3rd argument what about better changing or find what could be used as the 2nd argument (probably only the UNIX timestamp), If a different time zone is required the timestamp can be configured in advance and just pass it as an argument.

The idea with this is to make the parse method more "idempotent" if we could call it that way so that by using the cron expression and the timestamp should always return the same thing regarding the timezone (that if properly adjusted in the input should match)

What do you think?

Or probably better to have use something like:

pub fn parse<TZ: TimeZone>(cron: &str, dt: DateTime<TZ>) -> Result<DateTime<TZ>, ParseError> 
nbari commented 5 years ago

@jairinhohw please check the latest branch develop, is using

pub fn parse<TZ: TimeZone>(cron: &str, dt: DateTime<TZ>) -> Result<DateTime<TZ>, ParseError> 

So that you just need to pass the Datetime in your desired zone, please check it and let me know your thoughts.

Think I like is that still working as before with UTC:

parse("*/5 * * * *", Utc::now());

Or you could pass your desired timezone:

use chrono_tz::US::Pacific;
parse("*/5 * * * *", Utc::now().with_timezone(&Pacific));
jairinhohw commented 4 years ago

It seems much better, i didn't know you could get the timezone with this timezone method. That was the only reason i passed the tz argument.

jairinhohw commented 4 years ago

About the TODO in line 112, by looking at the implementation, it's impossible for the unwrap to fail.

nbari commented 4 years ago

hi @jairinhohw Regarding the line https://github.com/nbari/cron-parser/blob/develop/src/lib.rs#L175

   // Valid datetime for the timezone
   if let Some(dt) = tz.from_local_datetime(&next.naive_local()).latest() {

Why the usage of latest()?

jairinhohw commented 4 years ago

Is possible in some timezones (with DST) that one date occur more than one time, when the DST ends and the time goes back one hour. I feared that getting the earliest date it could cause an infinite loop in some instances by getting various dates using the result of the parse as de initial date of the new parse.

nbari commented 4 years ago

I see, the latest will use the second T from Ambiguous(T, T) right?

jairinhohw commented 4 years ago

Yes

nbari commented 4 years ago

Got it many thanks 👍