Adjusts windows local for millisecond and year #997
Conversation
src/offset/local/windows.rs
Outdated
| @@ -62,6 +63,7 @@ pub(super) fn naive_to_local(d: &NaiveDateTime, local: bool) -> LocalResult<Date | |||
| struct LocalSysTime { | |||
| inner: SYSTEMTIME, | |||
| offset: i32, | |||
| shifted: bool, | |||
There was a problem hiding this comment.
I think this should get a docstring to explain what it means.
src/offset/local/windows.rs
Outdated
| fn system_time_from_naive_date_time(dt: &NaiveDateTime) -> SYSTEMTIME { | ||
| SYSTEMTIME { | ||
| fn system_time_from_naive_date_time(dt: &NaiveDateTime) -> (SYSTEMTIME, bool) { | ||
| let (year, shifted) = if dt.year() < 1601 { |
There was a problem hiding this comment.
What if dt.year() is smaller than zero?
I also think we should shift year by some multiple of 400, to avoid getting into issues with leap year calculations. Ideally we'd cover this behavior with some testing.
There was a problem hiding this comment.
How low of a year does there need to be coverage for? We could go to the minimum year from naive internals, but it might need to be done iteratively due to the range. Or it can be cut off arbitrarily at a certain point.
There was a problem hiding this comment.
Would be nice to go all the way to the naive minimum. We can probably do some calculation to figure out how many multiples of 400 we should subtract?
src/offset/local/windows.rs
Outdated
| let date = | ||
| NaiveDate::from_ymd_opt(st.wYear as i32, st.wMonth as u32, st.wDay as u32).unwrap(); | ||
| let time = NaiveTime::from_hms(st.wHour as u32, st.wMinute as u32, st.wSecond as u32); | ||
| let year = if self.shifted { st.wYear - 1600 } else { st.wYear }; |
There was a problem hiding this comment.
Could value 1600 be a file-level const? Could a docstring explain the significance of year 1600?
Currently, 1600 is an opaque incantation.
src/offset/local/windows.rs
Outdated
| fn system_time_from_naive_date_time(dt: &NaiveDateTime) -> SYSTEMTIME { | ||
| SYSTEMTIME { | ||
| fn system_time_from_naive_date_time(dt: &NaiveDateTime) -> (SYSTEMTIME, bool) { | ||
| let (year, shifted) = if dt.year() < 1601 { |
There was a problem hiding this comment.
Could value 1601 value be a file-level const with a helpful docstring? Currently, value 1601 is an opaque incantation.
There was a problem hiding this comment.
There is a docstring down in SYSTEMTIME that designates the valid values for Window's SYSTEMTIME struct (1601-30827). I don't see the exact issue with a const though
src/offset/local/windows.rs
Outdated
|
|
||
| let offset = FixedOffset::east_opt(self.offset).unwrap(); | ||
| DateTime::from_utc(date.and_time(time) - offset, offset) | ||
| } | ||
| } | ||
|
|
||
| fn system_time_from_naive_date_time(dt: &NaiveDateTime) -> SYSTEMTIME { | ||
| SYSTEMTIME { | ||
| fn system_time_from_naive_date_time(dt: &NaiveDateTime) -> (SYSTEMTIME, bool) { |
There was a problem hiding this comment.
Is it possible to add test cases for only this function system_time_from_naive_date_time?
I can see this function is indirectly tested elsewhere but it seems good to me have tests focused solely on this function.
Placing the test within this file windows.rs wouldn't require changing the tested function's visibility.
src/offset/local/windows.rs
Outdated
| @@ -68,7 +68,12 @@ struct LocalSysTime { | |||
| inner: SYSTEMTIME, | |||
| /// The offset value from UTC | |||
| offset: i32, | |||
| // Denotes the +/- 400 shifted a year value from invalid to valid | |||
| // Denotes the multiple of 400 year intervals shifted to create a valid year value. The | |||
There was a problem hiding this comment.
Great comment. While you're at it, can this be a docstring, e.g. leading /// ?
|
Feedback changes made 😄 let me know what you think! |
|
@pitdicker Sounds good to me 😄 Definitely agree that #1017 would be the better approach as well |
This PR is meant to address a couple fixes/potential gaps in the last request that I found when running the most recent
0.4.xonboawith the test262 Date built-in suite.Primarily the changes are:
shiftedfield toLocalSysTime.