Corefxlab: Simplify name of the TimeOfDay struct

Created on 23 Oct 2017  路  14Comments  路  Source: dotnet/corefxlab

I suppose, it makes sense to rename the TimeOfDay struct to Time. Time is more simple and common name for that type, and there are no classes or namespaces called Time in .NET Core and .NET Core Lab (except the System.Time.Properties and System.Time.Tests namespaces, but they are internal) which will lead to name collisions.

Just a few standards where that type called time:

  • XSD time type
    <xs:element name='item' type='xs:time' minOccurs='0' />

  • HTML5 input type=time.
    <imput type="time" />

  • SQL time type (ISO/IEC 9075)
    CREATE TABLE test (a time);

area-System.Time

Most helpful comment

No, we do not need TimeOffset or DateOffset. I addressed why in #1740, so won't repeat here. (Just because we can, doesn't mean we should.)

Regarding DateTimeKind, no we don't need that either. One can ask what date it currently is in UTC or in the local time zone (or in any time zone), but once you have it as just a Date (year, month & day only) then any such reference to Kind becomes meaningless. Same goes for Time/TimeOfDay.

In other words, what would it actually mean for the date to be "2017-10-25 UTC"? It's not a useful concept (IMHO).

All 14 comments

The rationale for TimeOfDay name was:

  • To distinguish from TimeSpan - which is primarily intended for elapsed duration rather than 24-hour clock time.
  • To distinguish from the package name, System.Time - which is being used as an umbrella term rather than any referring to any particular component.

I'm fine with discussing / debating the merits of changing the struct name to just Time. Thank you for pointing out comparable references.

So, what do others think? +1 on the issue for yes, or -1 for no. I'll stay neutral for now. 馃槃

  • Yes, currently developers use TimeSpan to store time because they have no alternative. But TimeSpan has the Span suffix which means range or interval. Time has no suffix so it's just time without date part. The summary says that too. So developers should understand what is the Time structure and be happy to have it.
  • I don't think that umbrella is really needed, because the current problem is that TimeSpan is used to store time. So Time is the mostly wanted type from the package. The package description says Date and Time helper classes.

The name collision between the struct and the package is not a big problem, since there are no good alternatives except having ugly and/or long names, and the package name isn't used in code.

Maybe the types should be moved to .NET CLR. They are most wanted, and many things will depend on them. Then the problem with distinguishing from the package name will be eliminated.

So, the System.Time package is eliminated.

If there is agreement about merging to System.Runtime, then yes. Still, I'd like some other opinions here. If none, then I'll bring up the name options when pitching to the main corefx repo. Thanks.

Could call the package System.DateAndTime?

Time vs TimeSpan are fine, they both work as described

Or following on from https://github.com/dotnet/corefxlab/issues/1740#issuecomment-339273498

Why is there no support for the optional timezone designator in xs:time?

Could add both types?

Time - Timezone neutral time
TimeOfDay - Time including timezone

But what's about Date with timezone? TimeOffset and DateOffset would be more preferable because of the Offset suffix which DateTimeOffset has.

From https://github.com/dotnet/corefxlab/issues/1740#issuecomment-339273498

System.Date

  • Why is there no support for the optional timezone designator in xs:date?

System.TimeOfDay

  • Why is there no support for the optional timezone designator in xs:time?
public struct TimeOffset
{
    private Time _time;
    private short _offsetMinutes;

    private Time ClockTime => new Time((_time + Offset).Ticks, DateTimeKind.Unspecified);

    public Time Time => ClockTime;
    public TimeSpan Offset => new TimeSpan(0, (int)_offsetMinutes, 0);
}

public struct DateOffset
{
    private Date _date;
    private short _offsetMinutes;

    private Date ClockDate => new Date((_date + Offset).Ticks, DateTimeKind.Unspecified);

    public Date Date => ClockDate;
    public TimeSpan Offset => new TimeSpan(0, (int)_offsetMinutes, 0);
}

Also the DateTimeKind should be stored in Time and Date like in the DateTime struct (source). But for Date bits 31-32 should be used.

No, we do not need TimeOffset or DateOffset. I addressed why in #1740, so won't repeat here. (Just because we can, doesn't mean we should.)

Regarding DateTimeKind, no we don't need that either. One can ask what date it currently is in UTC or in the local time zone (or in any time zone), but once you have it as just a Date (year, month & day only) then any such reference to Kind becomes meaningless. Same goes for Time/TimeOfDay.

In other words, what would it actually mean for the date to be "2017-10-25 UTC"? It's not a useful concept (IMHO).

@tarekgh - Any objections to renaming TimeOfDay to Time?

WRT the offset question, I have reversed my thoughts on that. Please see #1901 for further.

I am still thinking yes to renaming the TimeOfDay type to Time. Unless others object, I will create a PR for this shortly. Thanks!

Any objections to renaming TimeOfDay to Time?

Only if this can break anyone already using the old name. If you are working to get this library to the core, I would say wait and rename it when you move it there.

@tarekgh - Yes, it would be a breaking change. However this is all still in pre-release. Everything on corefxlab is only on the private myget feed and the current version is 0.1.0-e171104-4. The semver spec allows major breaking changes while in 0.x, so I don't think that's an issue.

I seriously doubt anyone is actually using it beyond testing anyway.

ok, if you think this is not an issue, then I am fine too renaming it.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

tmds picture tmds  路  7Comments

MisinformedDNA picture MisinformedDNA  路  10Comments

ahsonkhan picture ahsonkhan  路  9Comments

KrzysztofCwalina picture KrzysztofCwalina  路  3Comments

KrzysztofCwalina picture KrzysztofCwalina  路  9Comments