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

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

Merged
merged 6 commits into from
Mar 22, 2024

Conversation

pitdicker
Copy link
Collaborator

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*.

Copy link

codecov bot commented Mar 16, 2024

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).

❗ 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 ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@djc
Copy link
Contributor

djc commented Mar 18, 2024

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
Copy link
Collaborator Author

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
Copy link
Contributor

djc commented Mar 18, 2024

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

@pitdicker
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

I'll collect some info in an issue.

@pitdicker
Copy link
Collaborator Author

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();

@pitdicker pitdicker mentioned this pull request Mar 20, 2024
/// let dt = NaiveDate::from_ymd(2018, 1, 26)
/// .unwrap()
/// .at_hms_micro(18, 30, 9, 453_829)
/// .unwrap()
/// .and_local_timezone(Utc)
/// .unwrap();
/// .and_utc();
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 weird if a later commit is just going to change it again to in_utc().

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 agree. This commit is merged on the main branch in #1530. I'll wait until main is merged into the 0.5.x branch again and then rebase this PR.

@pitdicker pitdicker merged commit 21ee9b7 into chronotope:0.5.x Mar 22, 2024
32 of 33 checks passed
@pitdicker pitdicker deleted the and_timestamp branch March 22, 2024 14:16
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

2 participants