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

Don't allow strange leap seconds in initialization methods #1283

Merged
merged 5 commits into from
Sep 12, 2023

Conversation

pitdicker
Copy link
Collaborator

As discussed in #944.

A leap second that is not on a minute boundary never happens in reality. It still makes sense to be able to represent them. But the less trivial it is to create them, the less confusion they can cause (?).

We have to add an extra check in NaiveTime::from_hms_nano_opt and NaiveTime::from_num_seconds_from_midnight_opt.

This also effects the following methods:

  • DateTime<Tz>::from_timestamp (brand-new 😄)
  • NaiveTime::from_hms_milli
  • NaiveTime::from_hms_milli_opt
  • NaiveTime::from_hms_micro
  • NaiveTime::from_hms_micro_opt
  • NaiveTime::from_hms_nano
  • NaiveTime::from_hms_nano_opt
  • NaiveTime::from_num_seconds_from_midnight
  • NaiveDate::and_hms_milli
  • NaiveDate::and_hms_milli_opt
  • NaiveDate::and_hms_micro
  • NaiveDate::and_hms_micro_opt
  • NaiveDate::and_hms_nano
  • NaiveDate::and_hms_nano_opt
  • NaiveDateTime::from_timestamp
  • NaiveDateTime::from_timestamp_opt
  • TimeZone::timestamp
  • TimeZone::timestamp_opt

A strange leap-second can still be created from an existing value by using a DateTime with a fractional minute offset, and by changing a time with Timelike::with_nanosecond.

@codecov
Copy link

codecov bot commented Sep 12, 2023

Codecov Report

Merging #1283 (8a64c8f) into 0.4.x (084b021) will decrease coverage by 0.09%.
Report is 8 commits behind head on 0.4.x.
The diff coverage is 81.08%.

@@            Coverage Diff             @@
##            0.4.x    #1283      +/-   ##
==========================================
- Coverage   91.41%   91.33%   -0.09%     
==========================================
  Files          38       38              
  Lines       16957    16971      +14     
==========================================
- Hits        15502    15501       -1     
- Misses       1455     1470      +15     
Files Changed Coverage Δ
src/datetime/mod.rs 86.08% <ø> (-0.53%) ⬇️
src/offset/mod.rs 50.66% <ø> (ø)
src/naive/date.rs 96.30% <50.00%> (-0.05%) ⬇️
src/naive/time/mod.rs 95.95% <72.41%> (-0.56%) ⬇️
src/naive/datetime/mod.rs 97.34% <100.00%> (-0.37%) ⬇️
src/naive/time/tests.rs 98.33% <100.00%> (-0.02%) ⬇️
src/round.rs 97.72% <100.00%> (+0.14%) ⬆️

... and 4 files with indirect coverage changes

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

@@ -861,8 +861,8 @@ impl NaiveDate {

/// Makes a new `NaiveDateTime` from the current date, hour, minute, second and millisecond.
///
/// The millisecond part can exceed 1,000
/// in order to represent the [leap second](./struct.NaiveTime.html#leap-second-handling).
/// The millisecond part is allowed exceed 1,000,000,000 in order to represent a [leap second](
Copy link
Member

Choose a reason for hiding this comment

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

"allowed exceed" -> "allowed to exceed" for all of these?

Nit: please try to keep the first line of your commit messages within ~72 characters.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, fixed.

@@ -458,7 +458,7 @@ impl NaiveTime {
#[inline]
#[must_use]
pub const fn from_num_seconds_from_midnight_opt(secs: u32, nano: u32) -> Option<NaiveTime> {
if secs >= 86_400 || nano >= 2_000_000_000 {
if secs >= 86_400 || (nano >= 1_000_000_000 && secs % 60 != 59) || nano >= 2_000_000_000 {
Copy link
Member

Choose a reason for hiding this comment

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

I would order the leap second conditional last here, since it is the most complex.

@pitdicker pitdicker force-pushed the from_hms_nano_opt branch 2 times, most recently from 5dba138 to d05eb11 Compare September 12, 2023 13:14
@pitdicker pitdicker changed the title Don't allow strange nanoseconds in initialization methods Don't allow strange leap seconds in initialization methods Sep 12, 2023
@pitdicker
Copy link
Collaborator Author

I forgot the arbitrary feature, let me take another look

@pitdicker pitdicker merged commit 6665804 into chronotope:0.4.x Sep 12, 2023
36 of 37 checks passed
@pitdicker pitdicker deleted the from_hms_nano_opt branch September 12, 2023 15:27
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.

2 participants