chronotope / chrono

Date and time library for Rust
Other
3.3k stars 523 forks source link

Rename `TimeZone::timestamp*` to `TimeZone::and_timestamp*` #1523

Closed pitdicker closed 6 months ago

pitdicker commented 6 months ago

We have the pattern to use the name and_ for methods that add information to convert to a more complete type. NaiveDate had and_time and and_hms*, NaiveDateTime has and_local_timezone.

This makes the methods on TimeZone consistent. TimeZone::timestamp* is renamed to TimeZone::and_timestamp* and TimeZone::with_ymd_and_hms to TimeZone::and_ymd_and_hms*.

codecov[bot] commented 6 months ago

Codecov Report

Attention: Patch coverage is 99.64158% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 93.95%. Comparing base (3317fd1) to head (fa27e5c).

:exclamation: Current head fa27e5c differs from pull request most recent head f80253c. Consider uploading reports for the commit f80253c to get more accurate results

Files Patch % Lines
src/datetime/tests.rs 98.34% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## 0.5.x #1523 +/- ## ========================================== - Coverage 93.97% 93.95% -0.02% ========================================== Files 37 37 Lines 16686 16672 -14 ========================================== - Hits 15680 15665 -15 - Misses 1006 1007 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

djc commented 6 months ago

So we may have this and_() pattern but I feel like with_() is much more common in the Rust ecosystem. Maybe we should be moving in the other direction instead? (I think that was the idea behind with_ymd_and_hms(), at least.)

Can we do some part of this on main?

pitdicker commented 6 months ago

Fine by me. Then we should probably rename the current with_ methods such as with_year to something like replace_year? That matches time-rs.

djc commented 6 months ago

How are the semantics for current with_() methods different from those for current and_() methods, in your opinion?

pitdicker commented 6 months ago

Currently the with_() methods return the same type and replace some part of the date, time or offset. The and_() methods add more information to turn it into another type.

pitdicker commented 6 months ago

Other methods that start with and are NaiveDate::{and_time, and_hms*}, NaiveDateTime::{and_local_timezone, and_utc}.

And one more that starts with with is DateTime::with_timezone.

pitdicker commented 6 months ago

I'll collect some info in an issue.

pitdicker commented 6 months ago

Added commits to rename all methods from #1527 except TimeZone::{from_local_datetime, from_utc_datetime}.

I am not 100% conviced with the new names for NaiveDateTime::{and_local_timezone, and_utc}. Naming them in_ does match natural language. And assume_ seems like saying the user may not be sure for some reason.

Example code:

let pst = FixedOffset::east(8 * 60 * 60)?;
// in_timezone
let dt = NaiveDate::from_ymd(2018, 1, 11)?.at_hms(10, 5, 13)?.in_timezone(pst);
// assume_timezone
let dt = NaiveDate::from_ymd(2018, 1, 11)?.at_hms(10, 5, 13)?.assume_timezone(pst);

// in_utc
let timestamp = NaiveDate::from_ymd(1970, 1, 1)?.at_hms(0, 0, 0).in_utc().timestamp();
// assume_utc
let timestamp = NaiveDate::from_ymd(1970, 1, 1)?.at_hms(0, 0, 0).assume_utc().timestamp();