Don't allow strange leap seconds in initialization methods #1283
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ 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
... and 4 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
pitdicker
force-pushed
the
from_hms_nano_opt
branch
from
259473e to
27d90ad
Compare
djc
reviewed
Sep 12, 2023
src/naive/date.rs
Outdated
| @@ -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]( | |||
There was a problem hiding this comment.
"allowed exceed" -> "allowed to exceed" for all of these?
Nit: please try to keep the first line of your commit messages within ~72 characters.
src/naive/time/mod.rs
Outdated
| @@ -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 { | |||
There was a problem hiding this comment.
I would order the leap second conditional last here, since it is the most complex.
pitdicker
force-pushed
the
from_hms_nano_opt
branch
2 times, most recently
from
5dba138 to
d05eb11
Compare
pitdicker
changed the title
|
I forgot the |
djc
approved these changes
Sep 12, 2023
pitdicker
force-pushed
the
from_hms_nano_opt
branch
from
d05eb11 to
8a64c8f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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_optandNaiveTime::from_num_seconds_from_midnight_opt.This also effects the following methods:
DateTime<Tz>::from_timestamp(brand-new 😄)NaiveTime::from_hms_milliNaiveTime::from_hms_milli_optNaiveTime::from_hms_microNaiveTime::from_hms_micro_optNaiveTime::from_hms_nanoNaiveTime::from_hms_nano_optNaiveTime::from_num_seconds_from_midnightNaiveDate::and_hms_milliNaiveDate::and_hms_milli_optNaiveDate::and_hms_microNaiveDate::and_hms_micro_optNaiveDate::and_hms_nanoNaiveDate::and_hms_nano_optNaiveDateTime::from_timestampNaiveDateTime::from_timestamp_optTimeZone::timestampTimeZone::timestamp_optA strange leap-second can still be created from an existing value by using a
DateTimewith a fractional minute offset, and by changing a time withTimelike::with_nanosecond.