Apply the TimeProvider design feedback

[tarekgh]

Fixes #36617
Fixes #84274

This is the third and last part of the changes to TimeProvider.

[dotnet-issue-labeler]

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, 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]

#84235
#83604

[ghost]

Tagging subscribers to this area: @dotnet/area-system-datetime
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fixes #84274
Fixes #36617

This is the third and last part of the changes to TimeProvider.

Author:	tarekgh
Assignees:	tarekgh
Labels:	new-api-needs-documentation, area-System.DateTime, needs-area-label
Milestone:	-

[pinkfloydx33]

I know that #84274 decided to go with methods for the "now" properties.... but it feels Java-y and was quite weird to look at upon review.

Maybe it's because it's not consistent with a simple DateTime.UtcNow invocation. That's what it feels like it should be matched to, especially since a "now" abstraction was the driving force behind the original request. Explaining why one is a method and the other isn't--especially to a newcomer--feels like it could crop up a bit. I understand the reasoning, it just feels too inconsistent with the current static properties.

Leaving it as a UtcNow property may also help keep the diffs tidy for many of the places migrating from their own ISystemClock implementations, for whatever that is worth.

Just my 2cp

[davidfowl]

I agree with leaving the properties as properties and not changing it to methods on this new abstraction. Feels weird to try and “fix things” here.

[stephentoub]

Feels weird to try and “fix things” here.

Why is it weird? This is a new abstraction. Why shouldn't we get it "right", especially when overrides can do even more than the erroneous-as-a-property UtcNow? I think the arguments made in the issue where this was discussed at length are pretty compelling. We shouldn't be relitigating it here.

(That said, I continue to be fine with it either way, but I do prefer the methods.)

[davidfowl]

I think I favor consistency over being “technically correct”. I also think about the future blog posts that continue to explain why there are differences and try to get the readers to really appreciate that mistakes were fixed. I won’t die on this hill, but I feel like these would have made a huge difference earlier and now it won’t have the same positive impact on user code (but it’ll make the design cleaner)

[stephentoub]

From a consistency perspective, I think consistency with itself is more important. There are similar explanation challenges as to why it's appropriate for UtcNow/LocalNow to be properties but GetTimestamp() to be a method.

[tarekgh]

There are convincing strong reasons mentioned in the following comments to have them as methods.

#36617 (comment)
#36617 (comment)

Additionally, users keep complaining having UtcNow and Now as a property and even we got requests before to obsolete such Now. I don't think we need to repeat the same mistake just to be consistent.

[davidfowl]

Sure, like I said, I wouldn’t die on this hill 😅

[niemyjski]

Question: Why would you want to throw null here? I'd expect NotImplementedException / abstract implementation.

[stephentoub]

This is the reference assembly. It should never be used at runtime, and the goal is to define just the signatures while keeping the size as small as possible. throw null is what we do in the ref assemblies everywhere.

[niemyjski]

Thanks, that makes sense!