Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Export TzOffset #117

Merged
merged 1 commit into from
Apr 5, 2024
Merged

Export TzOffset #117

merged 1 commit into from
Apr 5, 2024

Conversation

mjdwitt
Copy link
Contributor

@mjdwitt mjdwitt commented Nov 21, 2022

This type is included in the Tz's implementation of TimeZone and not exporting it makes it difficult to work with Tz in wrapper types. Are there changes planned for this type that preclude it from being exported or is this okay?

@mjdwitt
Copy link
Contributor Author

mjdwitt commented Nov 21, 2022

This question was brought up before in #35 but the asker closed it there after finding a workaround for their use-case.

@djc
Copy link
Member

djc commented Nov 28, 2022

I became the maintainer for this crate a few months ago and haven't had time to dig in all that much beyond some version bumps. Thus, I don't have any context in my head around TzOffset and I'm wary of exposing more API surface. However, if you can give more context on your use case and on the role you think TzOffset plays in this crate we could discuss a way forward.

@LucioFranco any opinions?

@mjdwitt
Copy link
Contributor Author

mjdwitt commented Dec 9, 2022

@djc Your wariness makes sense. I don't personally have any rush for this as I'm happy with using my branch of chrono_tz for now.

My specific use case is building a wrapper type that unifies Tz with a "floating" time zone for an implementation of iCal. It's not on GitHub, but it looks something like this:

/// iCal times and datetimes can either have a specific time zone or be considered "floating", i.e.
/// assumed to be in the current local timezone. Specific time zones can be either UTC or some
/// named time zone such as "America/New York". This enum models specific time zones as a wrapped
/// [`chrono_tz::Tz`] and floating time zones are just a [`chrono::Local`] under the hood.
#[derive(Copy, Clone, PartialEq, Eq, Debug)]
pub enum Zone {
    Floating,
    Named(chrono_tz::Tz),
}

This enum type is fine but the trick is in implementing chrono::TimeZone:

impl TimeZone for Zone {
    type Offset = ZoneOffset;
    // ...
}

#[derive(Copy, Clone, PartialEq, Eq, Debug)]
pub enum ZoneOffset {
    Floating(chrono::FixedOffset),
    Named(chrono_tz::TzOffset),
}

impl Offset for ZoneOffset {
    fn fix(&self) -> chrono::FixedOffset {
        match self {
            Self::Floating(offset) => *offset,
            Self::Named(tzoffset) => tzoffset.fix(),
        }
    }
}

Using my branch via a [patch.crates-io] directive in Cargo.toml lets me build this wrapper type by simply delegating to the underlying types for each variation of the wrapper.

Copy link
Contributor

@pitdicker pitdicker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me. There is no reason it should be private, seems like an oversight.

We currently document TzOffset as part of our public API via the TimeZone trait, the OffsetComponents and OffsetName traits. But the type itself doesn't show up in the documentation.

B.t.w. I wonder why we even have OffsetComponents and OffsetName traits, as all they do is add some methods to TzOffset. Maybe their only reason for existence is that the methods would not be discoverable on a private TzOffset?

@pitdicker pitdicker added this pull request to the merge queue Apr 5, 2024
Merged via the queue into chronotope:main with commit c28fd9c Apr 5, 2024
4 checks passed
@pitdicker
Copy link
Contributor

Thank you @mjdwitt!

@mjdwitt mjdwitt deleted the export-tzoffset branch August 4, 2024 23:39
@mjdwitt mjdwitt restored the export-tzoffset branch August 4, 2024 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants