Introduce DateOnly and TimeOnly types by tarekgh · Pull Request #50980 · dotnet/runtime

tarekgh · 2021-04-09T00:01:30Z

ghost · 2021-04-09T00:01:34Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

tarekgh · 2021-04-09T00:03:43Z

CC @mattjohnsonpint

tarekgh · 2021-04-09T00:10:00Z

CC @safern

src/libraries/System.Private.CoreLib/src/Resources/Strings.resx

src/libraries/System.Private.CoreLib/src/System/DateOnly.cs

src/libraries/System.Runtime/ref/System.Runtime.cs

src/libraries/System.Private.CoreLib/src/Resources/Strings.resx

src/libraries/System.Private.CoreLib/src/System/DateOnly.cs

src/libraries/System.Private.CoreLib/src/System/Globalization/DateTimeFormat.cs

src/libraries/System.Private.CoreLib/src/Resources/Strings.resx

src/libraries/System.Private.CoreLib/src/System/DateOnly.cs

gewarren

There is a lot of /// to review - here is some initial feedback.

src/libraries/System.Private.CoreLib/src/System/DateOnly.cs

src/libraries/System.Private.CoreLib/src/System/TimeOnly.cs

src/libraries/System.Private.CoreLib/src/System/DateOnly.cs

src/libraries/System.Private.CoreLib/src/System/Globalization/DateTimeFormat.cs

Added a temorary logging info to the exception for CI failure diagnostics

src/libraries/System.Private.CoreLib/src/System/TimeOnly.cs

src/libraries/System.Private.CoreLib/src/Resources/Strings.resx

src/libraries/System.Private.CoreLib/src/System/TimeOnly.cs

src/libraries/System.Private.CoreLib/src/Resources/Strings.resx

src/libraries/System.Private.CoreLib/src/System/Globalization/DateTimeFormat.cs

src/libraries/System.Runtime/tests/System/TimeOnlyTests.cs

tarekgh · 2021-04-12T23:42:34Z

Thanks @stephentoub & @danmoseley for all valuable feedback. I believe I have addressed all of it. Please let me know if you have any more comments or we are good to go.

danmoseley · 2021-04-13T01:27:19Z

I have no other comments but I did not review it all so I defer to @stephentoub

src/libraries/System.Private.CoreLib/src/System/TimeOnly.cs

Co-authored-by: Santiago Fernandez Madero <safern@microsoft.com>

tarekgh · 2021-04-13T20:35:57Z

@safern thanks for the feedback. do you have any more comments?

safern

LGTM

src/libraries/System.Private.CoreLib/src/System/TimeOnly.cs

src/libraries/System.Runtime/tests/System/TimeOnlyTests.cs

stephentoub · 2021-04-14T15:40:23Z

src/libraries/System.Private.CoreLib/src/System/DateOnly.cs

+            ParseFailure,
+            WrongParts,
+            BadFormatSpecifier
+        }


We can't share the same enum that's used with DateTime?

Also, indentation is off here. And if we can't share it, it should probably be a nested enum under DateOnly rather than being System.ParseOperationResult.

I tried to avoid sharing that with DateTimeParse.ParseFailureKind enum because they include different things specific to DateTime parsing and I wanted to avoid merging more values there that are not related to DateTime. I didn't make this enum nested inside DateOnly because I used it in TimeOnly too. we can still move it and reference it with the DateOnly but I thought this will be ugly.

For the rest of the comments, I'll try to address this in other PR later.

I wanted to avoid merging more values there that are not related to DateTime

I think it's fine to add more. DateTime, DateTimeOffset, DateOnly, and TimeOnly are all logically related and part of the same system, sharing many similar errors, share format and styles, etc.

stephentoub · 2021-04-14T15:41:00Z

src/libraries/System.Private.CoreLib/src/System/DateOnly.cs

+
+            return new DateOnly(newDayNumber);
+
+            static void ThrowOutOfRange() => throw new ArgumentOutOfRangeException(nameof(value), SR.ArgumentOutOfRange_AddValue);


This can't be on ThrowHelper rather than its own method?

stephentoub · 2021-04-14T15:42:52Z

src/libraries/System.Private.CoreLib/src/System/DateOnly.cs

+                    default:
+                        Debug.Assert(result == ParseOperationResult.WrongParts);
+                        throw new FormatException(SR.Format(SR.Format_DateTimeOnlyContainsNoneDateParts, s.ToString(), nameof(DateOnly)));
+                }


This block of exception handling code is duplicated in multiple places. It should instead be a helper method, just as with DateTime. That not only saves on duplication, it makes the call site more streamlined, with rarely used code separated out, makes it more likely to be inlineable, etc.

stephentoub · 2021-04-14T15:43:52Z

src/libraries/System.Private.CoreLib/src/System/DateOnly.cs

+        /// <returns>true if the s parameter was converted successfully; otherwise, false.</returns>
+        public static bool TryParse(string s, IFormatProvider? provider, DateTimeStyles style, out DateOnly result)
+        {
+            result = default;


This result = default; should be moved into the if block below.

stephentoub · 2021-04-14T15:46:37Z

src/libraries/System.Private.CoreLib/src/System/DateOnly.cs

+                                bool b = DateTimeFormat.TryFormatDateOnlyO(value.Year, value.Month, value.Day, destination);
+                                Debug.Assert(b);
+                            });
+                        }


Nit: these braces shouldn't be necessary... same for the 'r' case below

stephentoub · 2021-04-14T15:47:24Z

src/libraries/System.Private.CoreLib/src/System/TimeOnly.cs

+            ParseOperationResult result = TryParseInternal(s, provider, style, out TimeOnly timeOnly);
+            if (result != ParseOperationResult.Success)
+            {
+                switch (result)


Same comment as on DateOnly

stephentoub · 2021-04-14T15:47:55Z

src/libraries/System.Private.CoreLib/src/System/TimeOnly.cs

+                {
+                    case 'o':
+                    case 'O':
+                        {


truegoodwill · 2021-04-17T01:16:21Z

https://github.com/FastFinTech/FFT.TimeStamps

I pasted this link in case anyone's interested in code I wrote for my personal workaround for some of the the issues solved by this PR, plus a few more.

It has a DateOnly object, named as a DateStamp, for example, and a MonthStamp object as well. (Those objects are not used in any hotpaths, so instead of writing them from first principles, I wrapped DateTime objects with them, but then provided an interface that represents the intention of the objects.) It also has a TimeOfWeek object that represents ideas like "10am on Monday".

The main emphasis of this library has a couple of features that are not addressed by this PR:

Comparison of times in multiple timezones is extremely efficient due to resolving all times back to a utc-based TimeStamp object. It's like converting everything in your code to DateTimeKind.Utc but without the ambiguity and without giving user code the opportunity to accidentally put or deserialize a DateTimeKind.Local time where DateTimeKind.Utc was expected.
Conversion of timezones is a lot faster than the conversion utilities provided by TimeZoneInfo. See the TimeZoneCalculator class. Conversion iterators for sequential timestamps is even faster by magnitudes.

Whilst DateOnly and TimeOnly ideas do solve some problems and are very much needed in the framework, an object that is unquestionably UTC time and quickly/easily converted between timezones is also needed (at least, by me). The library linked above provides this. For example, the TimeStamp object can add an absolute amount of time, or you can pass in a TimeZoneInfo object to the method and it will add the given amount of time accounting for timezone offset changes in the given timezone.

Thanks for reading!

@mattjohnsonpint, updated to help out with the confused face :) Thanks to my .net heros for your amazing work!

Introduce DateOnly and TimeOnly types

8528143

tarekgh added the area-System.Runtime label Apr 9, 2021

tarekgh added this to the 6.0.0 milestone Apr 9, 2021

tarekgh self-assigned this Apr 9, 2021

ghost added the new-api-needs-documentation label Apr 9, 2021

tarekgh mentioned this pull request Apr 9, 2021

Developers can benefit from enhanced Date and Time types and Timezone support #45318

Closed

4 tasks

sharwell reviewed Apr 9, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/Resources/Strings.resx Show resolved Hide resolved

sharwell reviewed Apr 9, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/DateOnly.cs Outdated Show resolved Hide resolved

sharwell reviewed Apr 9, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/DateOnly.cs Outdated Show resolved Hide resolved

sharwell reviewed Apr 9, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/DateOnly.cs Outdated Show resolved Hide resolved

stephentoub reviewed Apr 9, 2021

View reviewed changes

src/libraries/System.Runtime/ref/System.Runtime.cs Show resolved Hide resolved

Feedback 1

b06a8ca

iSazonov reviewed Apr 9, 2021

View reviewed changes

safern reviewed Apr 9, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/Resources/Strings.resx Outdated Show resolved Hide resolved

safern reviewed Apr 9, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/Resources/Strings.resx Show resolved Hide resolved

safern reviewed Apr 9, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/DateOnly.cs Outdated Show resolved Hide resolved

safern reviewed Apr 9, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/DateOnly.cs Outdated Show resolved Hide resolved

safern reviewed Apr 9, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/DateOnly.cs Show resolved Hide resolved

gewarren reviewed Apr 9, 2021

View reviewed changes

safern reviewed Apr 9, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/DateOnly.cs Outdated Show resolved Hide resolved

mattjohnsonpint reviewed Apr 9, 2021

View reviewed changes

safern reviewed Apr 9, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Globalization/DateTimeFormat.cs Outdated Show resolved Hide resolved

safern reviewed Apr 9, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Globalization/DateTimeFormat.cs Show resolved Hide resolved

safern reviewed Apr 9, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Globalization/DateTimeFormat.cs Show resolved Hide resolved

Address Feedback 2

40592c6

Added a temorary logging info to the exception for CI failure diagnostics

mattjohnsonpint reviewed Apr 10, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/TimeOnly.cs Outdated Show resolved Hide resolved

iSazonov reviewed Apr 10, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/Resources/Strings.resx Outdated Show resolved Hide resolved

stephentoub reviewed Apr 12, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/TimeOnly.cs Outdated Show resolved Hide resolved

stephentoub reviewed Apr 12, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/TimeOnly.cs Outdated Show resolved Hide resolved

stephentoub reviewed Apr 12, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/TimeOnly.cs Outdated Show resolved Hide resolved

stephentoub reviewed Apr 12, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/TimeOnly.cs Outdated Show resolved Hide resolved

danmoseley reviewed Apr 12, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/Resources/Strings.resx Outdated Show resolved Hide resolved

src/libraries/System.Private.CoreLib/src/Resources/Strings.resx Outdated Show resolved Hide resolved

danmoseley reviewed Apr 12, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Globalization/DateTimeFormat.cs Show resolved Hide resolved

danmoseley reviewed Apr 12, 2021

View reviewed changes

src/libraries/System.Runtime/tests/System/TimeOnlyTests.cs Outdated Show resolved Hide resolved

src/libraries/System.Runtime/tests/System/TimeOnlyTests.cs Outdated Show resolved Hide resolved

Feedback 5

2fb0643

safern reviewed Apr 13, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/TimeOnly.cs Outdated Show resolved Hide resolved

safern reviewed Apr 13, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/TimeOnly.cs Show resolved Hide resolved

safern reviewed Apr 13, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/TimeOnly.cs Show resolved Hide resolved

tarekgh and others added 2 commits April 13, 2021 09:47

Update src/libraries/System.Private.CoreLib/src/System/TimeOnly.cs

90ae81e

Co-authored-by: Santiago Fernandez Madero <safern@microsoft.com>

Feedback 6

381d588

safern approved these changes Apr 13, 2021

View reviewed changes

mattjohnsonpint reviewed Apr 13, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/TimeOnly.cs Outdated Show resolved Hide resolved

mattjohnsonpint reviewed Apr 13, 2021

View reviewed changes

src/libraries/System.Runtime/tests/System/TimeOnlyTests.cs Outdated Show resolved Hide resolved

Feedback 7

5ba16e8

tarekgh merged commit 94c13d0 into dotnet:main Apr 14, 2021

tarekgh deleted the DateAndTimeOnly branch April 14, 2021 15:34

stephentoub reviewed Apr 14, 2021

View reviewed changes

tarekgh mentioned this pull request Apr 14, 2021

Address the remaining comments from Date/TimeOnly PR #51261

Merged

runfoapp bot mentioned this pull request Apr 15, 2021

Exception in test infrastructure - System.InvalidOperationException: Collection was modified after the enumerator was instantiated #11063

Closed

iSazonov mentioned this pull request Apr 28, 2021

Add Time value type in System #25816

Closed

ghost locked as resolved and limited conversation to collaborators May 17, 2021

jeffhandley added area-System.DateTime and removed area-System.Runtime labels Nov 22, 2022


		return new DateOnly(newDayNumber);

		static void ThrowOutOfRange() => throw new ArgumentOutOfRangeException(nameof(value), SR.ArgumentOutOfRange_AddValue);

Conversation

tarekgh commented Apr 9, 2021

Uh oh!

ghost commented Apr 9, 2021

Uh oh!

tarekgh commented Apr 9, 2021

Uh oh!

tarekgh commented Apr 9, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gewarren left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tarekgh commented Apr 12, 2021

Uh oh!

danmoseley commented Apr 13, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tarekgh commented Apr 13, 2021

Uh oh!

safern left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

truegoodwill commented Apr 17, 2021 •

edited

Loading