chronotope / chrono-tz

TimeZone implementations for rust-chrono from the IANA database
Other
229 stars 55 forks source link

Allow `chrono_tz::Tz -> chrono::FixedOffset` #85

Closed jorgecarleitao closed 2 years ago

jorgecarleitao commented 2 years ago

Parsing offsets of the form +01:00 can be done with chrono alone. Parsing any other timezone must be done with chrono-tz.

When we want to support the former, and optionally (i.e. via a feature gate) support the latter, it would be very useful to be able to convert chrono_tz::Tz to chrono::FixedOffset because it avoids having to write a generic for it.

The example code:

pub fn utf8_to_timestamp_ns(
    array: &[&str],
    fmt: &str,
    timezone: String,
) -> Result<Vec<i64>> {
    let tz: Result<chrono::FixedOffset, String> = parse_offset(timezone.as_str());

    if let Ok(tz) = tz {
        Ok(utf8_to_timestamp_ns_impl(array, fmt, tz))
    } else {
        chrono_tz_utf_to_timestamp_ns(array, fmt, timezone)
    }

where chrono_tz_utf_to_timestamp_ns:

#[cfg(feature = "chrono-tz")]
fn chrono_tz_utf_to_timestamp_ns(
    array: &[&str],
    fmt: &str,
    timezone: String,
) -> Result<Vec<i64>> {
    let tz: Option<chrono_tz::Tz> = timezone.as_str().parse().ok();
    if let Some(tz) = tz {
        Ok(utf8_to_timestamp_ns_impl(array, fmt, tz))
    } else {
        Err(ArrowError::InvalidArgumentError(format!(
            "timezone \"{}\" cannot be parsed",
            timezone
        )))
    }
}

and

fn utf8_to_timestamp_ns_impl<T: chrono::TimeZone>(
    array: &[&str],
    fmt: &str,
    tz: T,
) -> Vec<i64>;

this code would be much simpler if utf8_to_timestamp_ns_impl was not a generic. This is a simple example, but I find myself writing more complex examples.

Suggestion, implement Into<chrono::FixedOffset> for chrono_tz::Tz to bridge both structs, thereby reducing the generic code over chrono::TimeZone.

jorgecarleitao commented 2 years ago

After thinking about this, this does not make sense because there may be multiple spans for a unique Tz with different offsets. Sorry for the noise.