-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Eliminate ambiguous date and time values #39816
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. |
... we've had Personally, I rather wish that we'd get a good, complete, first-party date/time library, like I proposed years ago. For one thing, it allows representations much closer to conceptual/semantic than the limited types you've proposed. Although the migration plan you've outlined here is essentially what I imagined. Note that |
While NodaTime is good, I think simplicity is more important.
The problem with As a default, e.g. a simple programming example for new programmers, displaying a file time, or clock, in the "local" time is probably easier to understand. Plus, A change to
I consider The NodaTime In contrast, to resolve While Maybe as a display value (user input/output) a zoned time is useful, but internally you would in most cases want to store something that represents an instance. Where you do use it, such as a calendaring app, you probably need all the individual parts - time, timezone, date (or even day of month separate from month). While maybe it could be useful, I think using what we already have ( |
...
Generally uses of date/time values are split into two buckets:
This cannot be emphasized enough - when dealing with future date/time values, you need (in almost all cases) to be working with the civil time. If I put a time in my calendar for an appointment, the civil time shouldn't change if the rules do. The primary value isn't the
Yes, but for most real-world use cases
nit: This can't display wrong, because the offset is stored. It can be recorded wrong, or estimated wrong, if the database isn't correct/up to date, but the display will reflect the "true" value.
.... I can't see any reason this would be beneficial. Note that date/time are not separate values for things like this, they represent a singular combined value. Note that two dates with the same "date" aren't guaranteed to share the same 24-hour period. In addition, your scheme ignores the fact that it would put the same day-of-month from every month+year combination into the same bucket, which seems strange - and will be in the wrong bucket if you're trying to find everything in "your" day-of-month, rather than "origin" day-of-month. If you're trying to make it faster to query in a database, you're going to have better results by querying with a dynamic range. |
cc: @tarekgh |
@tarekgh, were you able to take a look at this? Is it something we should look at moving forward around or is it something we can close as not planned at this point? |
@tannergooding we have already done some of this by exposing |
... which is mostly a problem due to |
@tarekgh - I don't believe this proposal was for obsoleting The clear example is |
from the proposal:
That is fine, I am not opposing that :-) |
Sure. To be more precise on the 6 steps proposed in this issue
|
Mostly I agree with you @mattjohnsonpint. for no. 2, do you have any candidate list for that? I am aware about |
Not off hand. Someone should do a comprehensive search. |
Great work with the progress on things like DateOnly (removing one of the reasons to use DateTime) Once steps 1-5 have been done, what is the reasoning for not Obsoleting DateTime (note that obsolete doesn't mean remove now, but does mean an intention to remove in the future). Is it because builds that have "treat warnings as errors" will fail? Or is it because you don't think that we could even remove DateTime, so marking it obsolete is misleading. Or is it for zoned operations, e.g. a future calendar agreement to meet in Brisbane at 06:00 on 01 March 2040, which should remain valid in civil time even if daylights savings rules change? In my experience this is generally better represented as separate columns for TimeOnly and then a separate DateOnly (or something more complex) because you might be applying different rules to them (not for a one off, but for something recurring like "last day of the month") |
This one. Because
If you're doing this sort of work, you'd be better off using NodaTime, as we currently lack a
Recurring events is probably best represented with |
The main reason I'm opposed is that With regard to the cited advice:
This is wrong in my opinion. We should not recommend any type to be the default. Rather, each type should be described and developers should be advised to pick the type that best fits their scenario. After all, we have 13 different numeric types to choose from, and we don't tell developers to consider one of them as the "default" numeric type. Sometimes your working with fractional values and need a |
I'm not sure it is even occasionally, as I struggle to come up with a scenario when you actually have a valid use for DateTime? The one I used to use was always to store date only, e.g. for calendaring type applications; but with DateOnly that is no longer the case. Is there any other valid use case for DateTime remaining? (i.e. that doesn't have a hidden implicit offset assumption)
I agree it is widely used. Removing it outright would create some friction; obsoleting it would create less, and help hide/prevent further use.
Probably not a bad idea. Double was a product of a time with limited computing resources where we had to accept imprecision in return for adequate performance. If decimal was the default then it would probably prevent a lot of bugs and mistakes. (Although in this case I think the performance for things like graphics rendering is probably still a significant enough consideration... although they usually aren't done in C# though) |
Yes, many - and they are almost always in the future. Here are some that are easy to remember:
The last one is particularly interesting. Say that we took the "prefer Consider also that on a global scale, time zones offsets and DST rules change somewhat frequently. While some countries are more stable than others, logically we still cannot be certain of what the time zone offset for any future event will be until we are actually at that moment in time. Sometimes changes happen with very little notice (see my blog post) - which can wreak havoc on pre-computed offsets or UTC equivalents. If I had to give one "rule of thumb" to follow, it would be to pick the type that most accurately stores the original values that you have. If you know the date, store the date. If you know the date and time, then store the date and time. If you know the time zone and it's not variable, then store the time zone ID. If you know the offset (because it is either fixed or has already come to pass), then store the offset. If you don't know these things, then don't invent them. Don't try to pre-determine an offset, or convert a future time to or from UTC based on data that might change. |
Oh, and I forgot to mention - just because one should use a Say, for example, that you stored Sure, if I called instead My point is, deprecating |
Thinking about this more, if we were to flag potentially problematic usage with an analyzer, I would flag I suppose I'd also flag the |
.... yeesss, although probably this gets turned into per-store date/time values pretty quickly.
I want |
Are you interested in submitting a proposal for that? It is in our radar, but we haven't gotten into it yet. |
Thanks for the expanded examples. Calendaring (future dates) is the one item that I had flagged, but usually for more complex scenarios like "Set an alarm for every Tuesday at 08:00", where you need a more complex type to store than just DateTime. In that scenario the single date time becomes a specialised version of the more complex schedule, i..e you probably wouldn't use the DateTime type. Maybe what we need is a FutureDateTime (not a serious suggestion) that has date and time, but does not have things like Now or implicit conversion to/from DateTimeOffset, or all those other things that cause problems. Obsoleting (or removing) all those functions from DateTime would satisfy me; and effectively make DateTime for the future calendar dates. So I guess it is not so much the DateTime struct itself that I dislike, but the operations like Now(), conversion, etc. Let's change step 6 to: I'm pretty sure removing/hiding DateTime.Now() would solve 95% of the times DateTime is used incorrectly. Interesting with the Maybe rather than default a more subtle wording is needed, such as: Usually you will be dealing with instants in time, such as "now", or when an event occurred such as a file change, log entry, or other historical record -- in these cases use the If you are doing calendaring or other future date operations, consider the |
The reason you don't want something like this is that then it makes your code dependent on the current time of execution, as opposed to using the same standard type all the time and writing methods that take a parameter for the "current" time.
.... Personally, you should actually ban all the static |
Pretty much all my code uses ISystemClock from Microsoft.AspNetCore.Authentication. And code reviews check for any static use of Now, UtcNow, or DateTime and flag them as wrong. Many applications use "current time", e.g. for recording logs or timestamps, and very few use calendaring / future dates. Even then, calendaring functions usually require something more complicated than a single datetime, e.g. checking a recent project it has a crontab like schedule + offset (for the schedule) ... although it could just as easily be crontab + timezone. The calendar specification then gets turned into/checked against DateTimeOffset for when to run, with no need to use the DateTime struct itself. But as mentioned above, I realise it is not so much the struct I want to stop being used, but Now, implicit conversion, etc; sure they might sometimes work by coincidence -- i.e. keeping in the same timezone with no code changes -- but the code is inherently unsafe if you put on a different server/machine, or call methods in a different order, e.g. d.ToLocalTime() may get a different result than d.ToUniversalTime().ToLocalTime(), depending on the original value, which is weird. |
I hope this is still pursued. While the ambiguity of I believe what was not discussed is the ambiguity or rather the expressiveness of But for me as a dev, more specific minimalistic types (like Also I can see library authors somewhat struggling with As a side note, if a type like |
These two requirements are in conflict, Pedantically, a timestamp type like this is ignorant to all timezones, they're irrelevant to the thing being talked about. UTC is commonly chosen because it's well known and "safe". A pure seconds-since serialization is also common. |
I agree, maybe wanting a different value displayed, than is stored is wanting to much, or misleading. So that is definitely no requirement for me. But What is usually required is to record a point in time, irrespective of the time zone or offset that the server which originally created the timestamp had. And at the moment the runtime gives me only two somewhat ambiguous options to do that: a What I am proposing, because I believe it would make things simpler, more expressive and because I value type safety. Is only having one definitive option, a specific Personally I believe |
aka The Date project
Background and Motivation
Dotnet Core contains several types for representing dates and times. As noted in the guidance documentation, developers should "consider
DateTimeOffset
as the default date and time type for application development" (https://docs.microsoft.com/en-us/dotnet/standard/datetime/choosing-between-datetime)However in many cases, in examples, component packages, and business software,
DateTime
is used, even though it is the incorrect type -- part of this is historical becauseDateTime
existed prior toDateTimeOffset
. This issue even affects Dotnet and many examples from Microsoft, whereDateTime
is often used whereDateTimeOffset
would be more appropriate.This proposal - Everywhere that an ambigious
DateTime
has been used in Dotnet, it should be supplemented with an unambiguousDateTimeOffset
, and these values should be promoted as the default for future use.It is currently difficult to follow the guidance advice and use
DateTimeOffset
when basic classes likeFileInfo
only useDateTime
.There are still some uses for
DateTime
. For some, such as working with UTC dates and times,DateTimeOffset
can also be used, and should still be considered the default, or for working with times only thenTimeSpan
can be used.The cases where
DateTime
is the correct class are:working with dates only, for example a birthdate for calculating something like drinking age only the date part is relevant -- it is generally not relevant which timezone you were born in or your current timezone.
working with abstract dates and times, for example in a calendar application; a common example is having a meeting at 09:00 on the first of each month, where the instance of time will vary in line with timezones. In these cases the date and time components need to be treated separately.
In both these cases, only the date part is used, not the full
DateTime
, and would be better served by aDate
only structure; for all other cases eitherDateTimeOffset
or another type (TimeSpan
) is more appropriate. In some cases (e.g. calendar) both aDate
andTimeSpan
value would be needed, but also need to be handled independently (and not combined into a singleDateTime
value).Promoting the use of
Date
, for date only, andDateTimeOffset
, for other scenarios, will help eliminate the issues that can arise from using ambiguous dates, e.g. where a field is supposed to be date only bug a bug has introduce a time component into theDateTime
field -- such a bug is not possible with a date only structure.The long term goal is to eventually be able to mark
DateTime
asObsolete
-- something that I have proposed before.Proposed API
The new API changes would consist of adding a
Date
struct, and addingDateTimeOffset
properties through Dotnet to supplement where there is currently only aDateTime
.To implement unambiguous single points of time across the entire Dotnet is a large, long term project, that can be done incrementally, with the following roadmap:
DateTime
with a time component, where only date is needed.A basic structure can be introduced, and then incrementally added to as needed.
DateTimeOffset
properties anywhere thatDateTime
is used in Dotnet. This would allow the guidance of "considerDateTimeOffset
as the default" to be acted upon.In some cases these values may already exist, e.g.
FileSystemInfo
already usesDateTimeOffset
internally but just doesn't expose it in the API.Searching the code for something like "[\s.()=+-/!&[]{}]DateTime[\s.()=+-/!&[]{}]" within .cs files gives an idea of the scope -- 10,000 references across 1,000 files.
DateTimeOffset
onDateTime
. InternallyDateTimeOffset
usesDateTime
, plus the offset. This should be changed to a ulong, and makeDateTimeOffset
a stand alone structure without dependency onDateTIme
limited to conversions.Initially, this would involve duplication of some code from
DateTime
toDateTimeOffset
(there is also duplication in some places likecomdatetime
). Further incremental changes may reverse the dependency, e.g.DateTime
could call anIsValidTimeWithLeapSeconds
inDateTimeOffset
, to then remove the duplication.FileInfo
should be updated to show examples usingDateTimeOffset
fields, not date time.Steps 1-4 can be done in parallel, incrementally.
DateTime
is used can be incrementally markedObsolete
and/or hidden from editors (e.g. Intellisense), via[EditorBrowsable(EditorBrowsableState.Never)]
.Note that marking something
Obsolete
is not a directly breaking change, the API is the same and does not break any runtime dependencies. There may, however, be some indirect issues when recompiling any dependencies if "treat warnings as errors" is turned on -- these would be good indicators to fix those dependencies to also use theDateTimeOffset
alternative. You could potentially start this before steps 1-4 are complete, but it may be clearer to wait.DateTime
itself can be markedObsolete
, and/or hidden, having been fully replaced byDateTimeOffset
and, in some cases,Date
.There may be opposition to marking
DateTime
Obsolete
, due to the possibility of breaking build systems that have treat warnings as errors turned on (not a runtime breaking change), but if all instances within Dotnet have been replaced, and after sufficient time, this should be possible. Once this is achieved, ambiguous dates and times will have been eliminated from Dotnet.A compromise may be to hide the class from editors, which will not break builds but help encourage future developers follow the guidance and use
DateTimeOffset
.Note: I have previously raised a suggestion to mark
DateTime
asObsolete
.Usage Examples
The overview example of How to get information about files, folders, and drives, (https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/file-system/how-to-get-information-about-files-folders-and-drives) would be changed to use the
DateTimeOffset
version of the property.The corresponding current properties on
FileInfo
(FileSystemInfo
) areLastAccessTime
andLastAccessTimeUtc
. The specific pattern used forDateTimeOffset
properties can be discussed and agreed.Other possible variations could be
LastAccess
,LastAccessTimeAt
,LastAccessedAt
, orLastAccessedTimeAt
. Another possible variation is a past tense formLastAccessed
similar to theIFileInfo
interface fromMicrosoft.Extensions.FileProviders
, although this could be confused with the conventions for event names.Alternative Designs
The main well known alternative is probably Noda Time (https://nodatime.org/).
Risks
There is probably a lot of opposition to trying to replace a core construct such as
DateTime
, although the professional guidance is that in most casesDateTimeOffset
is far more appropriate to use.Marking it
Obsolete
could also indirectly cause issues with build processes in some cases, although it does not break runtime API compatibility.The text was updated successfully, but these errors were encountered: