-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Introduce Date and Time only structs #49036
Comments
I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label. |
What is the behavior of the conversion operators re: No need for a conversion from NodaTime uses the name |
We have said no new |
These should be properties. Properties have better perf characteristics than readonly fields. |
public static implicit operator DateOnly(DateTime dateTime) { throw null; } // Truncate the time Operators losing information should be explicit. |
public DateOnly(int year, int month, int day, Calendar calendar) { throw null; }
public static int operator -(DateOnly d1, DateOnly d2) { throw null; } Presumably this returns the count of days, but this will hamstring us if we ever introduce a relative offset type (something containing year/month/day differences, like NodaTime's Also, should we add this operator: public static DateTime operator +(DateOnly d, TimeOnly t) { throw null; } public static readonly TimeOfDay MaxValue; I'm assuming this is public bool IsBetween(TimeOfDay t1, TimeOfDay t2) { throw null; }
public static TimeSpan operator -(TimeOfDay t1, TimeOfDay t2) { throw null; } // Always positive time span Both Java's and NodaTime's equivalent return negative time spans when
|
right, I am removing it.
That make sense. thanks. I'll update that. |
Unspecified.
No, DateTimeOffset is kind offset from UTC. Converting date or time to that would be strange as these not related to the time zones at all. Also, we didn't find any real scenario need that.
we didn't choose this name for 2 reasons. First, it can be confusing for users will assume it is related to local time zone. Second, we wanted a name that start with the word Date for discoverability. |
Yes, we'll not retain the calendar. This constructor is provided as a helper to create a Date from any other calendars dates. We can clarify the doc to be explicit about the behavior and not confuse it with the AddMonth.
Yes, this will return the number of days. The caller of this operator would see it return an integer number and if doesn't understand it would look at the doc. providing explicit method would suffer from the same issue. no? I mean if we have
This is interesting idea. will think about but initially it makes sense to me.
right. I got other comment this should be aa property too.
I assume it cannot be
IsBetween is different than
Will try to look at that. Thanks for the feedback. |
You are correct. Thanks! |
Might you point me to the resource where I can learn why this is the case or shortly elaborate? |
The ask was for from. So
What I meant was that we should have something like the following: public int YearsDifference(DateOnly value) { throw null; }
public int MonthsDifference(DateOnly value) { throw null; }
public int DaysDifference(DateOnly value) { throw null; } If I subtract two dates I want their relative difference (years, months, days), not their absolute difference (days only), which we need a separate type for.
That was more a musing than a desire, and we probably should stay consistent with the existing types. I would probably avoid having only
I'm unsure how to feel about this version. I clearly see what you're going for here, but I'm not sure it's a net benefit. Note that I personally would prefer constructing a full |
Static properties that just return a constant value are very easy for the JIT to inline, without special measures. readonly static fields are not as easy. They can be only inlined in certain cases. It requires either tiered compilation to kick in after the static constructor that initialized the field run. Or it requires a complex global analysis in AOT compiler that attempts to prove that the value of the readonly field can be assumed to be a constant. To further demonstrate the problem with static readonly field optimizations, consider a program like this: // This prints "0 1"! It will print "1 1" if you change One to be a static property.
using System;
class Program
{
static bool Dummy = Method();
static readonly int One = 1;
static bool Method()
{
Console.WriteLine(One);
return false;
}
static void Main(string[] args)
{
Method();
}
} |
It's important with time-of-day operations, because comparison can only be done within a single day. In other words, it is easy to test if "now" is between 1:00 and 3:00, but a lot more challenging to test if "now" is between 23:00 and 01:00, and even more challenging to have a single operation that can do both. In pseudocode, given a
You can see how I implemented it in the corefxlab prototype here: |
This would definitely be useful. Sometimes I run into weirdness where I go adding a TimeSpan to a DateTime and get unexpected results because I think the DateTime's time part has been set to midnight (so it can be a "pure" date), but it actually hasn't. Having an explicit Date type would solve these problems; such a type would not be possible to "pollute" with time of day data. |
On the usefulness/motivation front, this question was asked 10 years ago. There are lots of good comments in there, and a detailed reply from yours truly in 2016. In particular, I appreciate this comment very much:
|
Not to derail with bikeshedding the names 🙂 but I think it would be okay, possibly with a compiler update. There are already such conflicts with |
There's no conflict with
I couldn't find any interesting cases of just For Also it's worth noting that VB.Net has a date literal syntax - regardless of what names we pick, we should consider if such syntax should also be supported for either of the new types. Example: Dim d As [Date] = #8/13/2002#
Dim t as TimeOfDay = #12:14 PM# There are a lot of other places that use
There are some random projects that have In summary, I'm most concerned with name conflicts of objects (in OData and Graph) and with the |
Would it make sense to call the date-only part of a Or, you could call it I wonder if it would make sense to create a general purpose |
Any chance of implementing IXmlSerializable like the structs in corefxlab? This would allow for more scenarios in, for example, WCF. Why are there no TimeSpan addition and subtraction operators for Date(Only)? Of course, we'd probably have to reject a TimeSpan that does not represent a multiple of a day (or would lead to a negative date). But this is not different than having to reject adding or subtracting a TimeSpan that would lead to a negative time or a time that exceeds 24h in case of TimeOfDay, or is it? |
Where is @jskeet when you need him |
FWIW, I'm fine with |
Thanks @jskeet. I think your comment just ended an infinite bike shedding 😄 |
Why? This isn't like the implicit cast from |
My answer to that is "always use the new one" and "pretend the old one doesn't exist". Every since |
But it throws information away. A From https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/operator-overloads:
|
Do note that |
@jskeet No fair bringing the FDG into this. We can't a proper debate when there's black-letter law already settling the question. |
Eh. |
We're getting a bit off topic, me thinks. 😁 I agree |
The question was whether or not a Personally I think the answer is "Yes, but not as part of this effort". |
I know the issue is settled, but I still like to add one more data point to the naming discussion. VB isn't the only language to use As for time, in Ruby the So being more explicit with the names will help people coming over from other languages. |
The existence of that particular type in Java should be forgotten. It's been superseded twice, the more recent (and better) one being JSR310 with Java 8... in 2014. The only reason to be using it is in the case of backwards compatibility. |
This naming of DateOnly and TimeOnly or TimeOfDay is odd. The existing DateTime obviously contains both date and time. If we wand just a date, call it Date, not DateOnly. If we want just time, call it Time, not TimeOnly or TimeOfDay. Calling it TimeOfDay is kind of redundant. What else would it be time of? |
@kkohler2 I think you missed the 147 comments above detailing why they settled on those names 😅 |
@terrajobst No chance of having either an implicit operator or (preferably) a method to convert a DateOnly to DateTime? Having to use |
Please don't. Implicit casting should not add information. I've lost track of the number of bugs caused by implicitly casting from DateTime to DateTimeOffset. |
I agree. There's nothing saying that 00:00:00.000 is the time part of this implicit conversion. A DateOnly is just that, a pure date. People are free to make their own extension methods etc if they want this functionality, but it shouldn't be in the BCL. |
I am definitely not advocating an explicit or implicit cast, this was just how it was implemented in CoreFxLab (if I'm not mistaken). Personally, I see much more harm in losing the time component when you invoke |
That seems ridiculous. Why shouldn't I be able to get a date only value from a date and time value? I'm not "losing" any information. I'll just taking the subset of the available information that I care about. Throwing an exception here is just a land mine waiting for someone unfamiliar with the API to trigger. |
The CoreFxLab implementation I did was purely experimental. By contrast, the Also, at this point, if there are further changes desired, I suggest they get their own GitHub issues, as this one is now completed. |
Exactly. I'm locking this thread now, for that reason. |
Background and Motivation
.NET expose DateTime and DateTimeOffset for handling date and time. It has been requested multiple time to expose Date only and Time only structs. There are many scenarios users need to handle Dates without caring about time at all. And scenarios need to handle time without caring about Date. Users today try to work around that by using DateTime or DateTimeOffset but users need to truncate the parts they don’t need and want to ensure the remaining values is not affected. This is error prone as DateTime and DateTimeOffset also carry some time zone related things and it is not meant handle absolute date and time.
Also, the interop scenarios with components like ADO.Net, SQL Client,...et.c which having types for Date and Time only require the users to handle the truncation manually.
There are detailed discussions about such requested in the reported issues #14744, #1740 and #14089. Initially we have suggested to expose these features outside the core as a NuGet package. This is implemented in corefxlab https://github.com/dotnet/corefxlab/tree/master/src/System.Time. Users still requesting this functionality to be in core, and they wanted to avoid dependencies on third party libraries and make sense such functionality be in the core.
Here is some text picked up from different issues talking about the user scenarios:
Best names for these types would be
Date
andTime
but there is issue that VB already using these names and if we go with such names can conflict with VB.Proposed API
The text was updated successfully, but these errors were encountered: