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

Add more documentation to TimeZone #1068

Closed
wants to merge 1 commit into from

Conversation

pitdicker
Copy link
Collaborator

Split out from #1040.

Copy link
Contributor

@jtmoon79 jtmoon79 left a comment

Choose a reason for hiding this comment

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

Approve with nitpick comments.

/// A type implementing [`TimeZone`] can be used to create a [`DateTime`]:
///
/// Creating a [`DateTime`] from a [`NaiveDateTime`]:
/// - [`from_local_datetime`](#method.from_local_datetime) if the datetime is local.
Copy link
Contributor

Choose a reason for hiding this comment

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

... is in local timezone.

src/offset/mod.rs Outdated Show resolved Hide resolved
src/offset/mod.rs Outdated Show resolved Hide resolved
@@ -209,10 +209,118 @@ pub trait Offset: Sized + Clone + fmt::Debug {
fn fix(&self) -> FixedOffset;
}

/// The time zone.
/// The [`TimeZone`] trait is the backbone of chrono, allowing it to work with local time.
Copy link
Contributor

Choose a reason for hiding this comment

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

The first line should explain what it is, and doesn't need to repeat the name itself. "backbone of chrono" doesn't convey anything particularly useful to a reader trying to familiarize themselves with what it does, and "allowing it to work with local time" seems kinda vague.

Maybe something like "Represents a time zone to help distinguish between timestamps from different zones"?

Comment on lines +216 to +217
/// Most methods of this trait are deprecated, it is usually better or easier to work with the
/// methods on [`DateTime`] and [`NaiveDateTime`] instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Leading off with "Most methods of this trait are deprecated" seems like a bad way to explain the trait. Before "Using TimeZone" we should probably start with a more detailed description of what TimeZone is and what it contains.

/// [`from_offset`](#tymethod.from_offset) allows the type to be reconstructed from the associated
/// [`Offset`] type, which gets stored in a [`DateTime`].
///
/// ## Example
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'd prefer not to have an example for implementing TimeZone. Creating new TimeZone implementations should be pretty rare, and finding the implementations in the chrono code base and inspecting their source code is pretty straightforward, so it seems low value to have a bunch of code here.

Comment on lines +219 to +235
/// A type implementing [`TimeZone`] can be used to create a [`DateTime`]:
///
/// Creating a [`DateTime`] from a [`NaiveDateTime`]:
/// - [`from_local_datetime`](#method.from_local_datetime) if the datetime is in local time zone.
/// - [`from_utc_datetime`](#method.from_utc_datetime) if the datetime is in UTC time zone.
///
/// Creating a [`DateTime`] from a Unix timestamp in UTC:
/// - [`timestamp_opt`](#method.timestamp_opt)
/// - [`timestamp_millis_opt`](#method.timestamp_millis_opt)
/// - [`timestamp_nanos`](#method.timestamp_nanos)
///
/// Creating a [`DateTime`] from year, month, day, time components:
/// - [`with_ymd_and_hms`](#method.with_ymd_and_hms)
///
/// Parsing a [`DateTime`] from a string:
/// - [`datetime_from_str`](#method.datetime_from_str). Note that if the string contains an offset
/// from UTC, that offset must match the timezone at that point in time.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to mainly advertise the TimeZone methods. IMO we should be advertising the NaiveDateTime methods that convert into DateTime here as alternatives.

@pitdicker
Copy link
Collaborator Author

I'll give this another try in the future.

@pitdicker pitdicker closed this Mar 17, 2024
@pitdicker pitdicker deleted the timezone_doc branch March 17, 2024 20:57
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.

None yet

3 participants