-
Notifications
You must be signed in to change notification settings - Fork 267
fix: set last access time with unspecified date time kind #879
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
fix: set last access time with unspecified date time kind #879
Conversation
This fixes an iconsistent DateTime conversion in the LastAccessTimeUtc setter by adding explicit conversion to local time. This fixes a conversion error that occurred when a value was passed with DateTimeKind.Unspecified.
fgreinacher
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work @xtopaz!
I left two comments :)
| { | ||
| var mockFileData = GetMockFileDataForWrite(); | ||
| mockFileData.LastAccessTime = value; | ||
| mockFileData.LastAccessTime = value.ToLocalTime(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also do this for the other *TimeUtc properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure whether ToLocalTime is correct here. I think we rather want something like this: https://github.com/dotnet/runtime/blob/96d1c8e3ba4bc94afdcf15ee2896a3bc984820e7/src/libraries/System.Private.CoreLib/src/System/IO/FileSystemInfo.cs#L108
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fgreinacher Sorry for the late reply, just got back from vacation.
All other *TimeUtc properties already contain the ToLocalTime call. This way the behavior has been unified.
File.File.GetUtcDateTimeOffsetcannot be used because it is internal. However, this behaves analogously to the proposed implementation, since it takes Utc for DateTimeKind.Unspecified.
To be more precise, the preliminary conversion using ToLocalTime for values with DateTimeKind.Unspecified assumes UTC. The subsequent implicit conversion to DateTimeOffset then takes the time zone into account correctly. Thus the timestamps behave analogously to those in System.FileInfo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fgreinacher I've incorporated the recent changes from #875 and all test look fine.
|
Obsolete by #875. |
This fixes a missing
ToLocalTimeconversion in theMockFileInfo.LastAccessTimeUtcsetter, which leads to inconsistent results when dealing withDateTimevalues havingDateTimeKind.Unspecified.