Implement DateTimeOffset.TotalOffsetMinutes #78943
Conversation
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to 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. |
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
ghost
assigned 
src/libraries/System.Private.CoreLib/src/System/DateTimeOffset.cs
Outdated
Show resolved
Hide resolved
| @@ -1745,6 +1745,7 @@ public enum DateTimeKind | |||
| public int Second { get { throw null; } } | |||
| public long Ticks { get { throw null; } } | |||
| public System.TimeSpan TimeOfDay { get { throw null; } } | |||
| public int TotalOffsetMinutes { get { throw null; } } | |||
There was a problem hiding this comment.
What is the primary purpose of this new property? From the issue description, it seems like it's purely about performance? If so, do we have numbers to validate it is in fact meaningfully better than Offset in the scenarios it'll be used? And out of all the places we use Offset, should any be using this instead?
There was a problem hiding this comment.
I talk about it a bit here in the proposal.
It's been quite a long time- I don't have the exact numbers anymore unfortunately. There are a number of places in the code where we could use this.
There was a problem hiding this comment.
One interesting data point is that, at least in Azure scenarios, DateTimeOffsets are usually UTC. In the index link above there are a few places where we're comparing the Offset to TimeSpan.Zero.
There was a problem hiding this comment.
I would have liked to see this PR use the new API. Who's following up to do that?
There was a problem hiding this comment.
What do you mean by this PR use the new API?
There was a problem hiding this comment.
I searched the who runtime repo and the only files can use the new property are:
runtime\src\libraries\System.Formats.Cbor\src\System\Formats\Cbor\Writer\CborWriter.Tag.cs
runtime\src\libraries\System.Private.Xml\src\System\Xml\Core\XmlWriter.cs
runtime\src\libraries\System.ServiceModel.Syndication\src\System\ServiceModel\Syndication\Atom10FeedFormatter.cs
runtime\src\libraries\System.ServiceModel.Syndication\src\System\ServiceModel\Syndication\Rss20FeedFormatter.cs
If it is worth it, I can try to submit a PR for these.
There was a problem hiding this comment.
Thanks. I think if it's worth adding the property, it's worth using it, assuming it makes the usage simpler/faster/etc. I'm pushing on this as I do any time we add API because it helps to validate whether the API is designed correctly and actually worthwhile.
There was a problem hiding this comment.
Sorry if I merged early. I'll try to submit a PR for that in the first chance. Thanks for the follow up!
There was a problem hiding this comment.
Not a problem :) It's just one of my pet issues: vetting the APIs we add wherever possible by trying to use them as best as possible in the millions of lines of production library code we maintain.
Co-authored-by: Eirik Tsarpalis <[email protected]>
|
The CI failure is unrelated. merging it. |
Fixes #52081