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.

Sign up for GitHub

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

Implement DateTimeOffset.TotalOffsetMinutes #78943

Merged
merged 2 commits into from
Nov 29, 2022
Merged

Implement DateTimeOffset.TotalOffsetMinutes #78943

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0a151ae
Implement DateTimeOffset.TotalOffsetMinutes
tarekgh Nov 29, 2022
aac5580
Update src/libraries/System.Private.CoreLib/src/System/DateTimeOffset.cs
tarekgh Nov 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/libraries/System.Private.CoreLib/src/System/DateTimeOffset.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ public DateTimeOffset(int year, int month, int day, int hour, int minute, int se
}

// Constructs a DateTimeOffset from a given year, month, day, hour,
// minute, second, millsecond and offset
// minute, second, millisecond and offset
public DateTimeOffset(int year, int month, int day, int hour, int minute, int second, int millisecond, TimeSpan offset)
{
_offsetMinutes = ValidateOffset(offset);
Expand All @@ -161,7 +161,7 @@ public DateTimeOffset(int year, int month, int day, int hour, int minute, int se
}

// Constructs a DateTimeOffset from a given year, month, day, hour,
// minute, second, millsecond, Calendar and offset.
// minute, second, millisecond, Calendar and offset.
public DateTimeOffset(int year, int month, int day, int hour, int minute, int second, int millisecond, Calendar calendar, TimeSpan offset)
{
_offsetMinutes = ValidateOffset(offset);
Expand Down Expand Up @@ -417,6 +417,11 @@ public DateTimeOffset ToOffset(TimeSpan offset) =>

public TimeSpan Offset => new TimeSpan(0, _offsetMinutes, 0);

/// <summary>
/// Gets the total number of minutes representing the time's offset from Coordinated Universal Time (UTC).
/// </summary>
public int TotalOffsetMinutes => _offsetMinutes;

// Returns the second part of this DateTimeOffset. The returned value is
// an integer between 0 and 59.
//
Expand Down
1 change: 1 addition & 0 deletions src/libraries/System.Runtime/ref/System.Runtime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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; } }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/DateTimeOffset.cs,1bc976bb59f3dd6e,references

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have liked to see this PR use the new API. Who's following up to do that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by this PR use the new API?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if I merged early. I'll try to submit a PR for that in the first chance. Thanks for the follow up!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have opened #79107.

public System.DateTime UtcDateTime { get { throw null; } }
public static System.DateTimeOffset UtcNow { get { throw null; } }
public long UtcTicks { get { throw null; } }
Expand Down
23 changes: 23 additions & 0 deletions src/libraries/System.Runtime/tests/System/DateTimeOffsetTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1381,6 +1381,29 @@ public static void ToString_ParseExactSpan_RoundtripsSuccessfully()
Assert.Equal(expectedString, actual.ToString("u"));
}

[Theory]
[InlineData(5)]
[InlineData(-5)]
[InlineData(0)]
[InlineData(14 * 60)] // max offset
[InlineData(-14 * 60)] // min offset
public static void TotalNumberOfMinutesTest(int minutesCount)
{
DateTimeOffset dto = new DateTimeOffset(new DateTime(2022, 11, 12), TimeSpan.FromMinutes(minutesCount));
Assert.Equal(minutesCount, dto.TotalOffsetMinutes);
Assert.Equal(minutesCount, dto.Offset.TotalMinutes);
}

[Fact]
public static void TotalNumberOfMinutesNowTest()
{
DateTimeOffset dto = DateTimeOffset.UtcNow;
Assert.Equal(0, dto.TotalOffsetMinutes);

dto = DateTimeOffset.Now;
Assert.Equal(dto.Offset.TotalMinutes, dto.TotalOffsetMinutes);
}

[Fact]
public static void TryFormat_ToString_EqualResults()
{
Expand Down