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

Documentation for DateTime #1040

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pitdicker
Copy link
Collaborator

If the general direction of this documentation seems useful, I can write it for more types.

My idea is to give a tour of the available methods on a type, similar to std::Option.

Useful examples could also be a plus, but this is just a start.

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.

This is great!

Feedback is recommended but not required.

src/datetime/mod.rs Outdated Show resolved Hide resolved
src/datetime/mod.rs Outdated Show resolved Hide resolved
src/datetime/mod.rs Outdated Show resolved Hide resolved
src/datetime/mod.rs Show resolved Hide resolved
src/datetime/mod.rs Outdated Show resolved Hide resolved
@pitdicker
Copy link
Collaborator Author

Thank you for the good suggestions!

I will add new commits to address review comments, and can squash them before later.

@pitdicker pitdicker force-pushed the datetime_doc branch 6 times, most recently from f774318 to 52cbb17 Compare May 16, 2023 05:22
Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

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

Let's split this in one PR per type. Please squash any review feedback into the relevant commits.

src/datetime/mod.rs Outdated Show resolved Hide resolved
@@ -79,11 +79,141 @@ pub enum SecondsFormat {
__NonExhaustive,
}

/// ISO 8601 combined date and time with time zone.
/// `DateTime` is the preferred type for working with real-world timestamps. It contains all the
/// information needed to exactly specify a point in time, and to do calculations on it: a date,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure it is correct to say that DateTime "contains" a "timezone". It contains the offset, but not the timezone itself. Maybe that can be worded differently?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure how to word it better. Changed it to "and associated time zone" for now. But the paragraph below explains that the time zone is tracked by the type system.

Copy link
Contributor

Choose a reason for hiding this comment

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

As I explained, my beef is with the word "contains". The next paragraph goes into a lot more detail, so I think we should get this first, high-level description right.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about "It carries all the information needed to exactly specify a point in time"?

src/offset/mod.rs Outdated Show resolved Hide resolved
/// - [`Sub<DateTime>`] to get a [`Duration`](struct.Duration.html) with the difference between two
/// datetimes.
/// - [`Add<Duration>`] to add a [`Duration`](struct.Duration.html) of an absolute number of
/// - [`signed_duration_since`](#method.signed_duration_since) to get a [`Duration`](
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 we should mention both the operator and the checked methods here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The line below says "The same functionality is also available with implementations of Add and Sub."
I initially made those more prominent, but they are not a great idea to use because they can panic in pretty common scenarios.

To make them a bit more visible I added an example of working with DateTimes.

@pitdicker pitdicker changed the title Documentation for DateTime and TimeZone Documentation for DateTime May 17, 2023
@pitdicker pitdicker force-pushed the datetime_doc branch 6 times, most recently from 1bde437 to 11723f5 Compare May 23, 2023 12:48
@pitdicker
Copy link
Collaborator Author

Finally updated. Sorry to let this linger for 3+ months.

@codecov
Copy link

codecov bot commented Sep 6, 2023

Codecov Report

Merging #1040 (d6b8bb4) into 0.4.x (604b92a) will decrease coverage by 0.06%.
The diff coverage is 60.71%.

@@            Coverage Diff             @@
##            0.4.x    #1040      +/-   ##
==========================================
- Coverage   91.50%   91.45%   -0.06%     
==========================================
  Files          38       38              
  Lines       17314    17341      +27     
==========================================
+ Hits        15844    15860      +16     
- Misses       1470     1481      +11     
Files Coverage Δ
src/datetime/mod.rs 88.15% <60.71%> (-1.15%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants