Skip to content
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

Add Today property to DateOnly #53498

Closed
simonziegler opened this issue May 31, 2021 · 23 comments
Closed

Add Today property to DateOnly #53498

simonziegler opened this issue May 31, 2021 · 23 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.DateTime
Milestone

Comments

@simonziegler
Copy link

Background and Motivation

The .NET 6 Preview 4 DateOnly struct lacks a Today property as found in DateTime

Proposed API

public static DateOnly Today => /*implementation*/;
@simonziegler simonziegler added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label May 31, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-Meta untriaged New issue has not been triaged by the area owner labels May 31, 2021
@ghost ghost added this to Needs triage in Triage POD for Meta, Reflection, etc May 31, 2021
@FilipToth
Copy link
Contributor

Ok, sounds nice except one problem: What time would we use, 12AM, or 12PM or a different time? Or null?

@huoyaoyuan
Copy link
Member

DateTime.Now.Date and DateTime.UtcNow.Date can differ.

@stefanloerwald
Copy link

stefanloerwald commented May 31, 2021

Why not DateOnly.Today => Date.FromDateTime(DateTime.Now.Date); and DateOnly.UtcToday => Date.FromDateTime(DateTime.UtcNow.Date);?

(probably the .Date part is superfluous. I haven't studied the API in detail)

@Symbai
Copy link

Symbai commented May 31, 2021

Why not DateOnly.Today => Date.FromDateTime(DateTime.Now.Date); and DateOnly.UtcToday => Date.FromDateTime(DateTime.UtcNow.Date);?

And why not using this directly? What's the real big major benefit from saving a word plus point in code (and maybe two brackets)?

@stefanloerwald
Copy link

Why not DateOnly.Today => Date.FromDateTime(DateTime.Now.Date); and DateOnly.UtcToday => Date.FromDateTime(DateTime.UtcNow.Date);?

And why not using this directly? What's the real big major benefit from saving a word plus point in code (and maybe two brackets)?

One word: convenience. DateOnly is essentially a convenience wrapper, so adding one certainly very common use case doesn't hurt, even if the implementation is stupidly trivial.

@simonziegler
Copy link
Author

I agree wit @stefanloerwald. Reduction of boilerplate is the key thing here. And having convenience functions also helps people who are still developing their C# coding skills, rather than forcing them to understand the subtle relationship between DateTime and DateOnly. This helps to complete the job already started.

@FilipToth
Copy link
Contributor

I agree wit @stefanloerwald. Reduction of boilerplate is the key thing here. And having convenience functions also helps people who are still developing their C# coding skills, rather than forcing them to understand the subtle relationship between DateTime and DateOnly. This helps to complete the job already started.

Sure but, is that worth adding another property to the DateTime class? Plus, we can already do DateOnly.Today. I don't think it's good design "merging" DateTime and DateOnly... Violates the Solid design principle, in my opinion.

@stefanloerwald
Copy link

I agree wit @stefanloerwald. Reduction of boilerplate is the key thing here. And having convenience functions also helps people who are still developing their C# coding skills, rather than forcing them to understand the subtle relationship between DateTime and DateOnly. This helps to complete the job already started.

Sure but, is that worth adding another property to the DateTime class? Plus, we can already do DateOnly.Today. I don't think it's good design "merging" DateTime and DateOnly... Violates the Solid design principle, in my opinion.

DateOnly does not have Today yet, only DateTime does, but the property is (obviously) of type DateTime. The proposal is not to add a property to DateTime, but to DateOnly.

Interestingly, the mentioned issue of "does Today mean today in local timezone or in UTC?" is not addressed in DateTime. There it's always local time zone.

@FilipToth
Copy link
Contributor

DateOnly does not have Today yet, only DateTime does, but the property is (obviously) of type DateTime. The proposal is not to add a property to DateTime, but to DateOnly.

Oh, my bad.
In that case, it would be inconsistent for us NOT to add this.

@ghost
Copy link

ghost commented Jun 3, 2021

Tagging subscribers to this area: @tannergooding
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and Motivation

The .NET 6 Preview 4 DateOnly struct lacks a Today property as found in DateTime

Proposed API

public static DateOnly Today => /*implementation*/;
Author: simonziegler
Assignees: -
Labels:

api-suggestion, area-System.Runtime, untriaged

Milestone: -

@joperezr joperezr removed this from Needs triage in Triage POD for Meta, Reflection, etc Jun 3, 2021
@ufcpp
Copy link
Contributor

ufcpp commented Jun 4, 2021

I prefer DateOnly not to depend on time zone.

@danmoseley
Copy link
Member

Cc @tarekgh

@tarekgh tarekgh removed the untriaged New issue has not been triaged by the area owner label Jun 4, 2021
@tarekgh tarekgh added this to the Future milestone Jun 4, 2021
@tarekgh
Copy link
Member

tarekgh commented Jun 4, 2021

@ufccp is right here. We are trying to keep DateOnly not relate to time zones in general. That is by design. That is why we didn't expose Today, Now, and UtcNow in DateOnly/TimeOnly. I am not seeing using Date.FromDateTime(DateTime.Today); or Date.FromDateTime(DateTime.UtcNow.Date); is a problem to use here as it would be more explicit what you are getting.

@tarekgh
Copy link
Member

tarekgh commented Jun 4, 2021

CC @mattjohnsonpint

@adamjones1
Copy link

A major reason I see in favour of adding these properties is for discoverability and adoption. IMO having a Date type (or Time type) is a huge step forward in modelling and developers should be encouraged to use DateOnly as a first-class citizen over DateTime whenever they're dealing with a date. As part of that experience I would expect to see an operation as common as getting the current date via Today/UtcToday readily available in the Intellisense drop-down on the type - needing to go via DateTime (using its own Today which shouldn't even be there) and to have to look up the FromDateTime method to convert it leaves a bad taste in the mouth and doesn't make it feel like it's on an equal to footing to DateTime.

I would still use it but I could imagine for a lot of folk who don't see the problem with using midnight to represent dates that this impression and any friction with using it would lead them to quickly fall back on just using midnight DateTimes (and the time zone issues that inevitably follow!).

Also, while it makes sense to decouple date instances from time zones, I would argue it's a different matter to decouple static members on the type (the factories) from time zones. It's unfortunate the C# team seem unwilling to consider static extension methods as they would solve this problem.

@stefanloerwald
Copy link

@ufccp is right here. We are trying to keep DateOnly not relate to time zones in general. That is by design. That is why we didn't expose Today, Now, and UtcNow in DateOnly/TimeOnly. I am not seeing using Date.FromDateTime(DateTime.Today); or Date.FromDateTime(DateTime.UtcNow.Date); is a problem to use here as it would be more explicit what you are getting.

I don't see how DateOnly.UtcToday is any less explicit than Date.FromDateTime(DateTime.UtcNow.Date);. In fact, I'd say they are identical in explicitness, but one is significantly more verbose and the other one is very expressive.

And also I don't see the issue with marginally timezone-aware utilities such as a Today property. It doesn't need anything else in DateOnly to be timezone-aware, so it is an isolated thing.

@tarekgh
Copy link
Member

tarekgh commented Jun 4, 2021

@stefanloerwald As I mentioned we are trying to keep DateOnly/TimeOnly not related to time zones in genral especially FromDateTime is simple enough to use for the concerned scenario.

@adamjones1 I want to clarify we are not exposing DateOnly/TimeOnly to encourage users not using DateTime/DateTimeOffset. Every type is addressing specific scenarios and it is up to the user to choose which type is best fit for the scenario need to achieve.

I want to ask here, what exactly the scenario you need DateOnly.Today/UtcToday/Now/UtcNow which using DateTime[Offset] wouldn't be enough for that scenario? Is it something nice to have for convenience? Or you think this is crucial to have to avoid other problems?

P.S. I am not trying to push back here on the proposal but want to understand the scenarios and reasoning.

@stefanloerwald
Copy link

I don't quite see the point of being so strict about time-zone-free code here. When you think about the concept of "today", you cannot at all think of it without also thinking about time zones. And when you think about dates, naturally there will be a point where you will want to relate it to "today", at least in many use cases.

So in my opinion, the goal of keeping the DateOnly code time-zone-free is limiting its usefulness. If you want to motivate people to use that new type, give it useful features. Today is one of them to me, and I don't seem to be alone with that.

@tarekgh
Copy link
Member

tarekgh commented Jun 4, 2021

I don't quite see the point of being so strict about time-zone-free code here.

Imagine we exposed Today/UtcToday. Then the next expected request we'll get is user will need to know if the DateOnly is created using local or Utc zones. This will be fair ask to have at that time. no? I am trying to avoid going this route and repeat the concerns we saw with DateTime.

So in my opinion, the goal of keeping the DateOnly code time-zone-free is limiting its usefulness.

Frankly, I disagree with that. We exposed Date/Time[Only] to address specific scenarios which would be extremely useful to use for. I am not sure why Today is going to limit the usage of such type?

@danmoseley
Copy link
Member

danmoseley commented Jun 4, 2021

When you think about the concept of "today", you cannot at all think of it without also thinking about time zones.

Which is why "today" is not a concept that fits well on a type that explicitly does not have a concept of time zones, IMO.

@mattjohnsonpint
Copy link
Contributor

I agree with Tarek, we shouldn't have such properties on DateOnly. Having them on DateTime and DateTimeOffset is arguably a mistake as well.

Instead, this is where a Clock class (or classes) would shine. Hypothetical example:

IClock clock = SystemClock.Local;
DateOnly date = clock.Today;

With such objects, we could have clocks that work with local time, UTC, or an arbitrary time zone, and properties that return any of DateOnly, TimeOnly, DateTime and DateTimeOffset. The interface would allow for mocking, and we could even obsolete the existing Now, UtcNow, and Today properties with message saying to use a clock instead.

@adamjones1
Copy link

I want to ask here, what exactly the scenario you need DateOnly.Today/UtcToday/Now/UtcNow which using DateTime[Offset] wouldn't be enough for that scenario? Is it something nice to have for convenience? Or you think this is crucial to have to avoid other problems?

It's a convenience and discoverability thing yep, I certainly don't think it's crucial. My concern is the disparity where it's trivial to obtain the current date in the local/UTC time zone in the 'wrong' representation for most use cases (I assert), but non-trivial to obtain it in the 'right' representation. My feeling is the API should nudge people towards using the best fitting model for their data, and typically a nominative instance in time isn't that for a calendar date.

I like @mattjohnsonpint's suggestion of getting both the current date and date-time via an IClock type though - that sounds much nicer than the status quo.

@tarekgh
Copy link
Member

tarekgh commented Jun 7, 2021

I am closing this issue as looks agreeing DateOnly is not the right type to expose the Today functionality. For @mattjohnsonpint suggestion, it is tracked already in the issue #36617. Thanks all for your feedback and discussion.

@tarekgh tarekgh closed this as completed Jun 7, 2021
@dotnet dotnet locked as resolved and limited conversation to collaborators Jul 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.DateTime
Projects
None yet
Development

No branches or pull requests