-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Proposal: System.Date type #14089
Comments
Wouldn't it be more appropriate for Beyond little details, though, I fully agree. There needs to be a type that conveys the concept of a date without time and vice versa. I actually like how Java 8 revamped their date/time libs so that date, time and zone are individual immutable components which can be combined into composite types representing DateTime, ZonedDateTime, etc. |
The idea is sound. In fact, you'll find a similar type called There are a few general problems to consider though:
There are other related date/time deficiencies that could be addressed at the same time - (without borrowing all of the ideas of Noda Time):
|
@HaloFour - Regarding |
Also - It might make sense to consider extracting all date/time related stuff to it's own Nuget package. ( |
@mj1856 I wouldn't name it |
@HaloFour I also think |
@michaelstaib I agree CalendarDate name will be confusing as it can indicate the date is related to some calendar (Japanese, Gregorian, Hijri...). |
@mj1856 Thanks for the detailed comments! Regarding VB, I assume an "Option" flag a la "Option Strict" is off the table? :-) Alternatively, couldn't we leave VB's Date keyword to mean System.DateTime, but allow them to do Imports Date = System.Date as a type alias? |
@tarekgh How do you feel about
|
@mj1856 CalendarDate will make sense if we are going to support all calendars with that type (as you have demonstrated in your code). I think this is not the intend from the original request to have a simple light weight Date type. I am seeing Date and CalendarDate can be used in different scenarios |
@michaelstaib - I don't think there's anything wrong with But with
That just can't work, because Even with |
@tarekgh - Ok. If the intention is to keep the type lightweight, meaning it's only field would be an integer representing the whole number of days since some epoch, then I understand why I actually like |
@mj1856 @michaelstaib Good point re: TimeSpan. Further reading: reference source of DateTime.AddYears/AddMonths: http://referencesource.microsoft.com/#mscorlib/system/datetime.cs,0b982f3a57816426,references |
I would strongly suggest we not re-invent the wheel here. The NodaTime project has already done a lot of work building a solid date/time API and working through the myriad of issues that naturally arise in such an effort. It incorporates many of the ideas from JodaTime and the newer date/time JSRs, while remaining idiomaticly .NET and actually improving on many of the flawed and overly complex Joda designs. |
@eatdrinksleepcode I'd be happy to see NodaTime in the BCL directly, like what Java 8 did by working with the Joda-Time developer. But having a separate library as a dependency just to have a calendar date type is painful. Opinion and rationale: If DateTime is in "mscorlib" or whatever the equivalent is with Core CLR, it feels like a calendar date type should be, too. Slippery slope acknowledged. |
@eatdrinksleepcode Also, isn't it true that the overwhelming majority of issues that NodaTime aims to solve (and the bugs they encounter) are related to times and time zones, rather than calendar dates? Calendar dates is a relatively easy problem to solve compared to times. |
As an active contributor to Noda Time, I would hope for better integration support rather than embedding it in the BCL. For example, it would be great if we could override the default mapping of SQL's At the same time, I think there's a valid need for some minimal expansion in the BCL also. @paulirwin - Noda Time does solve a lot of time zone related issues, but is also comprehensive around calendar dates. Jon Skeet is the primary author, and has done a ton of research into calendars - (even non-Gregorian calendars such as the Hebrew calendar). |
@mj1856 @paulirwin I also would prefere a simple and small calender type in the BCL which would solve the 80%. When I just want to map some date data from the database the type is just there at my fingertips. Whenever there is need for more one can opt for NotaTime ist just an install-package nodatime away ;). |
@mj1856 regarding the |
@paulirwin do you plan to provide the date type with similar formatting options like DateTime? |
@michaelstaib Yes, I would expect Date.LocalToday.ToString("MMMM d, YYYY") to work as well. The format string options would be a subset of the ones for DateTime, obviously removing the time-based ones. |
👍 Also, it should support I wonder if it's too late to consider remapping |
👍 As well as implement the same interfaces as DateTime: |
I think I'd prefer getting a complete, integrated date/time library (NodaTime, port of JSR310, or whatever), rather than little things piecemeal. Personally, I think that's what got us into some of the current mess in the first place - people adding things as they were requested, instead of a comprehensive whole. Date/time stuff is generally more complicated than people expect, with pieces related in occasionally strange ways. |
Date d2 = Date.UtcToday;
Date d3 = Date.LocalToday; Why does E.g.: Date date = Date.Today;
DateTime dateTime = date.ToLocalDateTime(); |
@svick - The Date object itself doesn't store the timezone, but to determine which day is "today" depends on the timezone. At 10pm Eastern Time tonight, LocalToday would be 2015-02-12 while UtcToday would be 2015-02-13. If Date.Today assumes "local" time, that leads to many of the same bugs that DateTime.Now causes. |
@paulirwin DateTime already have "Now" and "UtcNow" properties. so I think there shouldn't be any confusion with this one. may be you mean Now should be called LocalNow but I never see anyone confused when using "Now". DateTime.Today can be confusing if don't know it is always returns local date. In general I think having LocalToday and UtcToday would be a good idea as it is explicit. |
The SqlDataReader case @mj1856 outlines above (and note that SqlDataReader is used by ORMs like Entity Framework) was precisely my original motivation for creating this issue. I am not in favor of bloating the BCL for the sole reason of making more features available, but in this case, a calendar date is such a primitive function of so many business applications, and so many bugs and hacky workarounds exist to deal with the tacked-on time in DateTime, that a calendar date in the BCL would be beneficial in many aspects. I am also in favor of making things more open for extensibility for outside libraries like NodaTime, but "just use NodaTime instead" is inefficient (in the SqlDataReader case in particular), adds an extra dependency for just a fundamental chronological type, and requires significant wiring-up of that dependency for cases like deserialization of MVC parameters, for example. Also having parity between HTML5 input types and .NET types would be beneficial given that the overwhelming use of .NET is to create web-based applications. |
HTML5 parity is a very good point. Another point of parity mismatch is with W3C XML Schema Definition (XSD), which has Currently we can only use |
I do think that for the most part additional functionality should be filled-in by external libraries. But the CLR already provides an out-of-the-box implementation of chronology that is a fairly poor. Remaining in the BCL in their current state is not a pit of success, particularly for new developers. It also precludes the ability for other BCL assemblies from improving their use of chronology types. System.Data and System.Xml would benefit considerably from having better date/time/offset/timezone support, and those assemblies cannot take dependencies on NodaTime, and helper methods to bridge the two would be more likely to introduce bugs where they are not necessary. |
If I understand correctly, just adding an improved Date/Time API to core would not improve the SqlDataReader scenario at all; SqlDataReader would also have to be updated to understand the new API. It would be similar for any other existing functionality that currently maps to the existing Date/Time APIs; it would all have to be updated.
|
Correct, just adding the new types doesn't fix the situation with the other assemblies at all. What it does is open the door to updating those assemblies to support the new API officially. Since System.Data, System.Xml and the like cannot take dependencies on third-party polyfill libraries like NodaTime they cannot be improved without such underlying BCL support. |
@eatdrinksleepcode can't System.Data and System.Xml changed to adapt for custom conversions? I mean to make it work with any third party library through a type conversion pattern that the library can implement and register with System.Data and System.Xml |
@tarekgh Probably, but that would be inherently more complex, and anyone new to the framework are just going to see |
This thread is getting long. I would love to agree on some plan of action. Here is a straw man I would like to propose:
|
I'm curious as to how 2 would play out. In those APIs you often have specifically typed methods, e.g. Given that Noda seems to be the preferred date/time library (and that Java used Joda as a basis for their chronology enhancements) I wonder how it would feel to instead explore extending System.Data, System.Xml et al to support the Noda types directly as a strawman. |
Would it be helpful if I fork NodaTime, remove the parts that aren't in the scope of this issue, normalize the API against conventions, and present it as a reference for what a System.Date (and perhaps System.TimeOfDay) type could look like? |
@paulirwin I think this will be a good start here. if you share the initial type definition we can collect some design feedback too before finalizing the implementation. |
I have a near-complete implementation of |
I've posted the prototype to mj1856/corefx-dateandtime. It still needs lots more unit tests, but I think I got most of the desired functionality. It also needs to be optimized for performance. Currently it relies on For the sake of not spamming this thread - please just use that repo's tracker for any issues, suggestions, pull requests, etc. until such time which it might be merged in. We should keep this thread for general comments about the idea rather than my proposed implementation. |
Awesome work @mj1856! I will help out with your repo rather than trying to create my own prototype. I like the direction here. |
@mj1856 Thanks Matt for your effort. I took a quick look at the Date type and here is some notes and questions:
Thanks again Matt |
As @tarekgh mentioned on @mj1856's repo, the idea is that this issue should be closed until it is proven in the field. I did not pick up on that from earlier comments here but if that's how it should be handled then that's acceptable. Personally, I think that if there is an intention to implement this in the future, then the issue should be left open, perhaps given a future milestone or tag, because otherwise will be difficult to determine when, exactly, it is "proven" in the field. Is there a way to quantify that? Should I re-open this issue when I've deployed a single app to production successfully using @mj1856's library, or does it take X users/projects/months/stars/forks before it is proven? That's an honest question, I'm not trying to be inflammatory. I just really, sincerely care about this issue - the headaches having to shoe-horn calendar dates into DateTime has wasted numerous hours of my life - and I'd be sad if it falls by the wayside due to a "won't fix" closed status and I'd like to re-open it if it legitimately meets some criteria for being proven. If there is no objective criteria for this being proven, I'd say we should keep this issue open until it is given a definitive "fixed" or "won't fix" status. |
Also another reason for not closing the issue - if it is closed, someone else could come along, see no open issues for this, and create a new open issue, starting this conversation all over again. |
@paulirwin this means we cannot close any issue :-) when someone open same issue again we'll direct him/her to the old issue. this is regular process for any issue. they will not have to re-discuss it again. and even they will have the freedom to reopen the old issue |
Fair enough, then we can close the issue if that's the consensus here. But please either follow @mj1856's library closely going forward or let me know what the rough criteria would be for the library being considered proven. |
we should be in contact with @mj1856 moving forward anyway and we'll recommend this library for anyone run into to this issue. yes we should keep our eye on this library. Thanks Paul for your thoughts and bringing such issue. and thanks of course to Matt helping with the design and implementation |
Thanks @tarekgh! Yes - I'm fine with closing the issue for now. We'll keep iterating on mj1856/corefx-dateandtime. I'll also put together examples of corefx/coreclr changes that would be compelling use cases for should it be merged. |
Currently, trying to use System.DateTime to represent just calendar dates is overkill and is an easy way to introduce bugs. SQL Server supports a native date-only type, and having a date-only type in .NET to match would be very handy. But not only from SQL: accepting an MVC action method parameter from an HTML5 input type="date" field would be simplified with a native Date type.
Examples:
The text was updated successfully, but these errors were encountered: