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

Proposal: Add overload DateTime formatting APIs which takes TimeZoneInfo #19537

Open
steentottrup opened this issue Dec 2, 2016 · 26 comments
Open
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.DateTime
Milestone

Comments

@steentottrup
Copy link

Here is the summary of the proposal:

API Proposal

namespace System
{
    public sealed class DateTime : ValueType, IComparable, IComparable<DateTime>, IConvertible, IEquatable<DateTime>, IFormattable
    {
        public String ToLongDateString(TimeZoneInfo tzi)
        public String ToLongTimeString(TimeZoneInfo tzi)
        public String ToShortDateString(TimeZoneInfo tzi)
        public String ToShortTimeString(TimeZoneInfo tzi)
        public String ToString(TimeZoneInfo tzi)
        public String ToString(String format, TimeZoneInfo tzi)
        public String ToString(TimeZoneInfo tzi, IFormatProvider provider)
       public String ToString(String format, IFormatProvider provider)
    }
}

Details

Need a way to be able to format the dates using non-default/Local time zone. currently to do that will need manually convert the date to the target time zone and then format it.

Today if someone want to achieve the same result as the proposed APIs will write a code like

    var now = DateTime.Now;
    var zone = TimeZoneInfo.FindSystemTimeZoneById("Turkey Standard Time");
    var localTime = TimeZoneInfo.ConvertTime(now, zone);
    Console.WriteLine($"{localTime.ToLongDateString()}");

and will need to call the time zone conversion API TimeZoneInfo.ConvertTime every time before formatting the date/time.

Also today there is no easy way to format the DateTime with a target time zone using a formatting pattern like "o" which include the time zone information. for example doing:

    var now = DateTime.Now;
    var zone = TimeZoneInfo.FindSystemTimeZoneById("Turkey Standard Time");
    var localTime = TimeZoneInfo.ConvertTime(now, zone);
    Console.WriteLine($"{localTime.ToString("o")}");

will produce something like

2017-01-17T23:37:21.1371643

which either will show the formatted string without the time zone info or if the time zone info is there will be matching the local time zone or UTC time zone but cannot have the target time zone information.
The desired formatted string from the example should be something like

2017-01-17T23:37:21.1371643+03:00

where +03:00 reflect Turkey time zone offset from UTC.

the way the proposed APIs work will manually check the Kind of the DateTime, if it is Local/Unspecified then it will convert the DateTime from local time zone to target time zone. if the Kind is UTC, will convert it from UTC to target time zone.

If the output format is the one which include the time zone info (e.g. 'o' pattern), the resulted formatted string should reflect the target time zone.

Sample Code

    var now = DateTime.Now;
    var zone = TimeZoneInfo.FindSystemTimeZoneById("Turkey Standard Time");
    Console.WriteLine($"{now.ToLongDateString(zone)}");
@tarekgh
Copy link
Member

tarekgh commented Dec 5, 2016

@steentottrup we need API proposal for that. the proposal should address the details of how formatting dates will work with DateTime objects with (Local, UTC and Unspecified DateTimeKind). also the proposal need to address how this should work in the UWP apps as in UWP the current cultures for example are not the user cultures instead are driven by app + user profile cultures.

@steentottrup
Copy link
Author

steentottrup commented Dec 13, 2016

I'm sorry I have no experience (or knowledge) with UWP, but if cultures are driven by the app + user profile, I bet this will fit right in, as what I'm suggesting is a way to configure the timezone (for the UI).

Like with CultureInfo, the current timezone should be available somewhere from the context of a running application. In "old" dotnet you could get it on the thread or CultureInfo.CurrentUICulture etc. So i suggest the same with the TimeZoneInfo. Today I believe there's a Local property somewhere, that will give you the timezone selected by the administrator on the server (if you're running in a web app as an example).

So I suggest putting a "current" property on the TimeZoneInfo class, and like with CultureInfo, fetch a current TimeZone from the thread. It should probably be a CurrentUITimeZone, as my proposal is strickly for showing the timezone right in all formatted texts.

As is possible with CultureInfo, you should be able to change this property. It could default to the what the Local property is today, and it should probably be configurable in the app/web.config files, like CultureInfo is.

Then when it comes to actually formatting the date (and time), we need an method that will get the time (and date) when taking the selected timezone into account. Something like:

    public String ToLongTimeString() {
            Contract.Ensures(Contract.Result<String>() != null);
            return DateTimeFormat.Format(this.ToCurrentTimeZone(), "T", DateTimeFormatInfo.CurrentInfo);
    }

When doing the recalculation of the date/time, DateTimeKind should be taken into account, which should be easy for Local and UTC. I have no idea what to do with the Unspecified, which is a bit of a bastard in my world.

I hope this helped, please post feedback!

@tarekgh
Copy link
Member

tarekgh commented Dec 13, 2016

I believe we have 2 options to support formatting with TZ:

  • we can attach default TZ object to CurrentCulture (note that is not CurrentUICulture) and then all Date/Time formatting will use this TZ object during the formatting. the problem with this approach is for the app which don't want to use the TZ during the formatting even when someone (like any used library) set it. in addition to that, this can create app compat issues as the app can opt-in using new version of any library which starts to set TZ.
  • the other option is to introduce overloaded formatting methods (in DateTime and possibly DateTimeFormatInfo) which can accept the TZ object and format the Date accordingly. the app will have choice to format the dates with or without TZ and will have full control on the results. in this approach there is no notion of default TZ and instead the API can format with any giving TZ object. it is similar approach of the formatting APIs which takes a culture today.

I am inclining to the second option as it gives more control on the behavior and also it is more clearer for the app what to expect. in same time it reduces the risk of running issues (like in UWP, current culture is kind of global to the app and not limited to the thread).

@steentottrup
Copy link
Author

I was hoping for option one, and it's actually kind of what I did the first couple of times I needed timezones (putting it on the CultureInfo that is).
I hadn't taken into account the compat issue, where the app isn't interested in the tz support, but a library used, is changing the tz.

So option 2 is certainly acceptable! Even though I dislike the need for all these extra methods.

@steentottrup
Copy link
Author

steentottrup commented Dec 14, 2016

Just a small note to what you wrote on option 2,

the app will have choice to format the dates with or without TZ

The datetime format actually decides if the timezone should be part of the output or not.

So what I think you meant (?) was if the datetime should be recalculated to the given (as an argument) timezone, right? Or do you want to provide an argument to tell the method to pull the (configured) timezone from the current thread/cultureinfo and then recalculate?

@tarekgh
Copy link
Member

tarekgh commented Dec 14, 2016

my thought was to pass TZ object to the formatting and ask formatting the Date/Time using that TZ object.

The input DateTime will be converted to value matches the input TZ and then get formatted with this value. I think this is the scenario you want to achieve, right?

@steentottrup
Copy link
Author

It is, yes! The option to get the format output for a datetime with another tz than the one selected on the server by the admin.

@steentottrup
Copy link
Author

steentottrup commented Dec 16, 2016

So instead of what I did before, where the GetTimeZoneInfo method is some black magic/hack I did, where I stored a TimeZoneInfo for each request in my web app, using DI:

    public static DateTime ToLocalDateTime(this DateTime dt) {
        TimeZoneInfo tzi = GetTimeZoneInfo();
        dt = TimeZoneInfo.ConvertTimeFromUtc(dt, tzi);
        return dt;
    }

This is what it will look like if this proposal gets accepted and implemented:

    public struct DateTime {

        public String ToLongTimeString() {
            Contract.Ensures(Contract.Result<String>() != null);
            return DateTimeFormat.Format(this, "T", DateTimeFormatInfo.CurrentInfo);
        }

        public String ToLongTime(TimeZoneInfo tzi) {
            Contract.Ensures(Contract.Result<String>() != null);
            return DateTimeFormat.Format(
                TimeZoneInfo.ConvertTimeFromUtc(dt.ToUniversalTime(), tzi),
                "T",
                DateTimeFormatInfo.CurrentInfo
            );
        }
    }

And then where you call this method from, you'll need to get the current (default or configured or what ever) TimeZoneInfo from somewhere (the current thread or...).

@tarekgh
Copy link
Member

tarekgh commented Dec 16, 2016

introducing the proposed method make sense to me.

    public String ToLongTime(TimeZoneInfo tzi) {
        Contract.Ensures(Contract.Result<String>() != null);
        return DateTimeFormat.Format(
            TimeZoneInfo.ConvertTime(dt, tzi),
            "T",
            DateTimeFormatInfo.CurrentInfo
        );

notice I have changed a little bit in the implementation you previously provided.

The question now is, does it make sense to have overload to ToLongDateString, ToShortTimeString...etc. or not

@steentottrup
Copy link
Author

It always makes sense, on all DateTime format methods. Imagine the scenario where it's a forum, a user has posted a new topic at 2:00 AM UTC, on December 16th 2016, now you visit the site, you're in GMT minus something (bigger than 2 hours), so this needs to show as December 15th, 2016, even without a time and timezone in the output string.

At least I think so!

@tarekgh
Copy link
Member

tarekgh commented Dec 16, 2016

we need to have the exact proposal to the design review committee. Thanks.

@steentottrup
Copy link
Author

I'll make something for this!

@steentottrup
Copy link
Author

steentottrup commented Dec 22, 2016

What I would like, is for this piece of code, generating this output:

Posted by me, in Denmark, at Saturday, December 17, 2016 6:28:00 AM local time
That's UTC: Saturday, December 17, 2016 5:28:00 AM UTC
And displays as (UTC-07:00) Mountain Time (USA og Canada) time: Friday, December 16, 2016 10:28:00 PM

To look like this piece of code instead.

The changes being:

    Thread.CurrentThread.CurrentTimeZone = TimeZoneInfo.FindSystemTimeZoneById("Mountain Standard Time");

and

    postedDk.ToLongDateString(Thread.CurrentThread.CurrentTimeZone)

With this proposal, you can take any DateTime with a DateTimeKind of UTC or Local, and 'convert' it into a given TimeZone for output purpose.

@tarekgh
Copy link
Member

tarekgh commented Dec 22, 2016

why you need to have Thread.CurrentThread.CurrentTimeZone? this can be handled by the application, I am not seeing much value to attach CurrentTimeZone to the thread. you can achieve the same by defining ThreadStatic in your code already.

so I guess the proposal now is we just need to add the new API

    public string ToLongDateString(TimeZoneInfo tzi)

in DateTime struct.

@steentottrup
Copy link
Author

steentottrup commented Dec 23, 2016

Sorry, you're absolutely right, that line of code was from my original idea, where you didn't have a new method with the TimeZoneInfo argument, but where the methods themselves fetched that TimeZoneInfo off the current thread.

So the changes to the API would be adding a TimeZonInfo parameter to these methods (actually add another method with the parameter added):

        public String ToLongDateString()
        public String ToLongTimeString()
        public String ToShortDateString()
        public String ToShortTimeString()
        public override String ToString()
        public String ToString(String format)
        public String ToString(IFormatProvider provider)
        public String ToString(String format, IFormatProvider provider)

Right?

@tarekgh
Copy link
Member

tarekgh commented Jan 3, 2017

ok to summarize the signatures of the APIs we need to add to DateTime structure:

    public String ToLongDateString(TimeZoneInfo tzi)
    public String ToLongTimeString(TimeZoneInfo tzi)
    public String ToShortDateString(TimeZoneInfo tzi)
    public String ToShortTimeString(TimeZoneInfo tzi)
    public String ToString(TimeZoneInfo tzi)
    public String ToString(String format, TimeZoneInfo tzi)
    public String ToString(TimeZoneInfo tzi, IFormatProvider provider)
    public String ToString(String format, IFormatProvider provider)

@tarekgh tarekgh changed the title Proposal: Add TimeZoneInfo to the current context inline with CultureInfo Proposal: Add overload DateTime formatting APIs which takes TimeZoneInfo Jan 3, 2017
@karelz
Copy link
Member

karelz commented Jan 13, 2017

@tarekgh can you please summarize the final API in the top post? (here's typically used template)
BTW: I don't see class/namespace. Motivation/example would help as well.

@tarekgh
Copy link
Member

tarekgh commented Jan 13, 2017

@steentottrup I have moved your first comment to the next one and updated the first comment with the proposal.

@karelz please let me know if you have any other asks.

thanks

@karelz
Copy link
Member

karelz commented Jan 14, 2017

Thank you!

@terrajobst
Copy link
Member

We just took a brief look. For semantics, is it fair to say that your code sample above:

var now = DateTime.Now;
var zone = TimeZoneInfo.FindSystemTimeZoneById("Turkey Standard Time");
Console.WriteLine($"{now.ToLongDateString(zone)}");

is (logically) the same as:

var now = DateTime.Now;
var zone = TimeZoneInfo.FindSystemTimeZoneById("Turkey Standard Time");
var localTime = zone.ConverDateTime(now);
Console.WriteLine($"{localTime.ToLongDateString()}");

@steentottrup @tarekgh ?

@karelz
Copy link
Member

karelz commented Jan 17, 2017

We need more info and @tarekgh's expertise to do proper review. If you can update the top post with the background, alternatives, etc., that would be great. If not, we can discuss it next time with @tarekgh in the room.

@tarekgh
Copy link
Member

tarekgh commented Jan 17, 2017

@terrajobst @karelz

The mentioned code by @terrajobst is correct as if want to achieve the same results with the proposed APIs except in one case

var now = DateTime.Now;
var zone = TimeZoneInfo.FindSystemTimeZoneById("Turkey Standard Time");
var localTime = TimeZoneInfo.ConvertTime(now, zone);
Console.WriteLine($"{localTime.ToLongDateString()}");

Basically the APIs will just help in converting the date/time to the target time zone before the formatting so the caller don't have to manually convert to the target time zone.

The only difference between the proposed API and the alternative code is when formatting the date with the pattern which include the time zone info, the time zone info should reflect the target time zone and not the local time zone.

var now = DateTime.Now;
var zone = TimeZoneInfo.FindSystemTimeZoneById("Turkey Standard Time");
var localTime = TimeZoneInfo.ConvertTime(now, zone);
Console.WriteLine($"{localTime.ToString("o")}");

This will produce something like:

2017-01-17T23:37:21.1371643

while the new proposed API should produce something

2017-01-17T23:37:21.1371643+03:00

to reflect the target time zone. we don't have any way today to achieve this result without the proposed APIs.

@karelz
Copy link
Member

karelz commented Jan 17, 2017

Is extending the string formatting out of question?
It would be nice to include the background in motivation section of the proposal at the top.

@tarekgh
Copy link
Member

tarekgh commented Jan 17, 2017

@karelz

Is extending the string formatting out of question?

What you mean by that? do you mean the date/time formatting pattern strings? if this is the case I don't think this will help much as most people want to use the existing APIs without specifying the patterns.

It would be nice to include the background in motivation section of the proposal at the top.

I already mentioned the following in the details, what exactly else you want to see?

Need a way to be able to format the dates using non-default/Local time zone. currently to do that will need manually convert the date to the target time zone and then format it.

@karelz
Copy link
Member

karelz commented Jan 17, 2017

For me (with little background on specific APIs in the space), the sample code above is just much easier to comprehend - the alternative is to talk about it in person.

What you mean by that? do you mean the date/time formatting pattern strings? if this is the case I don't think this will help much as most people want to use the existing APIs without specifying the patterns.

I see localTime.ToString("o") in your sample, I assume the "o" is important there and does not produce the right format. Is there option to extend that? e.g. localTime.ToString("oo")?

@tarekgh
Copy link
Member

tarekgh commented Jan 17, 2017

@karelz I have added the alternative code sample in the proposal to be easier for the reader.

my point regarding the formatting string is will still not help much even if we added something like "oo" such pattern will help format the date in specific pattern but not cover other pattern. Also the callers prefer the APIs (something like DateTime.ToLongDateString(), ToShortDateString(), ToString()) instead of calling the format method with the date time pattern. only people care much about specific format will pass the needed pattern. in our case here we need to allow formatting the date with the target time zone in all possible currently supported formatting APIs and patterns.

if we introduced the new format like "oo" we'll still need to pass the target time zone anyway, so introducing a new format will not buy us anything.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@joperezr joperezr removed the untriaged New issue has not been triaged by the area owner label Jul 6, 2020
@joperezr joperezr modified the milestones: 5.0.0, Future Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.DateTime
Projects
None yet
Development

No branches or pull requests

8 participants