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

[API Proposal]: new System.ComponentModel.DateOnlyConverter and System.ComponentModel.TimeOnlyConverter classes #68743

Closed
0xced opened this issue May 1, 2022 · 8 comments · Fixed by #70057
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.ComponentModel
Milestone

Comments

@0xced
Copy link
Contributor

0xced commented May 1, 2022

Background and motivation

Converting between System.DateOnly or System.TimeOnly and string is currently not supported using a System.ComponentModel.TypeConverter. Many system types are supported out of the box (System.DateTimeOffset, System.Guid, System.TimeSpan, System.Uri etc.) so I think System.DateOnly and System.TimeOnly would be welcome additions.

I was expecting this to work, but converting from string actually throws:

System.NotSupportedException: TypeConverter cannot convert from System.String.

TypeConverter dateOnlyConverter = TypeDescriptor.GetConverter(typeof(DateOnly));
DateOnly? date = dateOnlyConverter.ConvertFromString("1940-10-09") as DateOnly?;
TypeConverter timeOnlyConverter = TypeDescriptor.GetConverter(typeof(TimeOnly));
TimeOnly? time = timeOnlyConverter.ConvertFromString("7:00") as TimeOnly?;

Having built-in support for type conversion would probably help to drive adoption of the relatively new DateOnly and TimeOnly type.

Addressing this issue would fix #59253 which is mentioned in System.Runtime work planned for .NET 7 (but only under the Backlog list, not the Planned for .NET 7 list).

API Proposal

namespace System.ComponentModel
{
    public class DateOnlyConverter : System.ComponentModel.TypeConverter
    {
        public DateOnlyConverter() { }
        public override bool CanConvertFrom(System.ComponentModel.ITypeDescriptorContext? context, System.Type sourceType) { throw null; }
        public override bool CanConvertTo(System.ComponentModel.ITypeDescriptorContext? context, [System.Diagnostics.CodeAnalysis.NotNullWhen(true)] System.Type? destinationType) { throw null; }
        public override object? ConvertFrom(System.ComponentModel.ITypeDescriptorContext? context, System.Globalization.CultureInfo? culture, object value) { throw null; }
        public override object? ConvertTo(System.ComponentModel.ITypeDescriptorContext? context, System.Globalization.CultureInfo? culture, object? value, System.Type destinationType) { throw null; }
    }

    public class TimeOnlyConverter : System.ComponentModel.TypeConverter
    {
        public TimeOnlyConverter() { }
        public override bool CanConvertFrom(System.ComponentModel.ITypeDescriptorContext? context, System.Type sourceType) { throw null; }
        public override bool CanConvertTo(System.ComponentModel.ITypeDescriptorContext? context, [System.Diagnostics.CodeAnalysis.NotNullWhen(true)] System.Type? destinationType) { throw null; }
        public override object? ConvertFrom(System.ComponentModel.ITypeDescriptorContext? context, System.Globalization.CultureInfo? culture, object value) { throw null; }
        public override object? ConvertTo(System.ComponentModel.ITypeDescriptorContext? context, System.Globalization.CultureInfo? culture, object? value, System.Type destinationType) { throw null; }
    }
}

API Usage

Using the converter would most likely happen through TypeDescriptor.GetConverter(typeof(…)):

var dateOnlyConverter = TypeDescriptor.GetConverter(typeof(DateOnly));
var date = dateOnlyConverter.ConvertFromString("1940-10-09") as DateOnly?;

var timeOnlyConverter = TypeDescriptor.GetConverter(typeof(TimeOnly));
var time = timeOnlyConverter.ConvertFromString("7:00") as TimeOnly?;

or even through a higher level abstractions such as Microsoft.Extensions.Configuration.Binder (which also uses TypeDescriptor.GetConverter internally) as described in #64739:

// Let's make the end of the world date configurable because we're not really sure about that date
var configuration = new ConfigurationBuilder()
    .AddInMemoryCollection(new Dictionary<string, string>
    {
        ["EndOfWorldDate"] = "2012-12-21",
        ["BreakfastTime"] = "7:00:00",
    })
    .Build();
var endOfTheWorldDate = configuration.GetValue<DateOnly>("EndOfWorldDate");
var breakfastTime = configuration.GetValue<TimeOnly>("BreakfastTime");

It could also be used by directly creating a DateOnlyConverter or TimeOnlyConverter instance but this is a less likely scenario.

Alternative Designs

To the best of my knowledge, there's no alternative design that would make sense for adding new System.ComponentModel.TypeConverter subclasses.

Risks

System.ComponentModel.DateOnlyConverter and System.ComponentModel.TimeOnlyConverter being a new classes, no risks are introduced.

@0xced 0xced added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label May 1, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.ComponentModel untriaged New issue has not been triaged by the area owner labels May 1, 2022
@ghost
Copy link

ghost commented May 1, 2022

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

Issue Details

Background and motivation

Converting between System.DateOnly and string is currently not supported using a System.ComponentModel.TypeConverter. Many system types are supported out of the box (System.DateTimeOffset, System.Guid, System.TimeSpan, System.Uri etc.) so I think System.DateOnly would be a welcome addition.

I was expecting this to work, but converting from string actually throws:

System.NotSupportedException: TypeConverter cannot convert from System.String.

TypeConverter dateOnlyConverter = TypeDescriptor.GetConverter(typeof(DateOnly));
DateOnly? date = dateOnlyConverter.ConvertFromString("1940-10-09") as DateOnly?;

Having built-in support for type conversion would probably help to drive adoption of the relatively new DateOnly type.

Addressing this issue would fix #59253 which is mentioned in System.Runtime work planned for .NET 7 (but only under the Backlog list, not the Planned for .NET 7 list).

Note that a similar proposal for the System.TimeOnly type has been filed as #68744.

API Proposal

public class DateOnlyConverter : System.ComponentModel.TypeConverter
{
    public override bool CanConvertFrom(System.ComponentModel.ITypeDescriptorContext? context, System.Type sourceType);
    public override bool CanConvertTo(System.ComponentModel.ITypeDescriptorContext? context, System.Type? destinationType);
    public override object? ConvertFrom(System.ComponentModel.ITypeDescriptorContext? context, System.Globalization.CultureInfo? culture, object value);
    public override object? ConvertTo(System.ComponentModel.ITypeDescriptorContext? context, System.Globalization.CultureInfo? culture, object? value, System.Type destinationType);
    public override bool IsValid(System.ComponentModel.ITypeDescriptorContext? context, object? value);
}

API Usage

Using the converter would most likely happen through TypeDescriptor.GetConverter(typeof(DateOnly)):

var dateOnlyConverter = TypeDescriptor.GetConverter(typeof(DateOnly));
var date = dateOnlyConverter.ConvertFromString("1940-10-09") as DateOnly?;

or even through a higher level abstractions such as Microsoft.Extensions.Configuration.Binder (which also uses TypeDescriptor.GetConverter internally) as described in #64739:

// Let's make the end of the world date configurable because we're not really sure about that date
var configuration = new ConfigurationBuilder()
    .AddInMemoryCollection(new Dictionary<string, string> { ["EndOfWorldDate"] = "2012-12-21" })
    .Build();
var endOfTheWorldDate = configuration.GetValue<DateOnly>("EndOfWorldDate");

It could also be used by directly creating a DateOnlyConverter instance but this is a less likely scenario:

var dateOnlyConverter = new System.ComponentModel.DateOnlyConverter();
var date = dateOnlyConverter.ConvertFromString("1940-10-09") as DateOnly?;

Alternative Designs

To the best of my knowledge, there's no alternative design that would make sense for adding a new System.ComponentModel.TypeConverter.

Risks

System.ComponentModel.DateOnlyConverter being a new class, no risks are introduced.

Author: 0xced
Assignees: -
Labels:

api-suggestion, area-System.ComponentModel, untriaged

Milestone: -

@tarekgh
Copy link
Member

tarekgh commented May 1, 2022

Could you please merge this proposal with the one in #68744? there is no need to track them in two different proposals. Thanks for filing the proposal.

@tarekgh tarekgh removed the untriaged New issue has not been triaged by the area owner label May 1, 2022
@tarekgh tarekgh added this to the 7.0.0 milestone May 1, 2022
@tarekgh tarekgh self-assigned this May 1, 2022
@0xced 0xced changed the title [API Proposal]: new System.ComponentModel.DateOnlyConverter class [API Proposal]: new System.ComponentModel.DateOnlyConverter and System.ComponentModel.TimeOnlyConverter classes May 1, 2022
@thomaslevesque
Copy link
Member

This would be very useful. Many components and libraries use TypeConverter internally. Microsoft.Extensions.Configuration.Binder, as mentioned by the OP, but also ASP.NET Core model binding, JSON.NET...

In the meantime, it's possible to make it work by adding the converter dynamically at runtime:

class DateOnlyConverter : TypeConverter
{
    public override bool CanConvertFrom(ITypeDescriptorContext context, Type sourceType)
    {
        if (sourceType == typeof(string))
            return true;
        return base.CanConvertFrom(context, sourceType);
    }

    public override bool CanConvertTo(ITypeDescriptorContext context, Type destinationType)
    {
        if (destinationType == typeof(string))
            return true;
        return base.CanConvertTo(context, destinationType);
    }

    public override object ConvertFrom(ITypeDescriptorContext context, CultureInfo culture, object value)
    {
        if (value is string s)
            return DateOnly.Parse(s, culture);
        return base.ConvertFrom(context, culture, value);
    }

    public override object ConvertTo(ITypeDescriptorContext context, CultureInfo culture, object value, Type destinationType)
    {
        if (destinationType == typeof(string))
            return ((DateOnly)value).ToString(culture);
        return base.ConvertTo(context, culture, value, destinationType);
    }

    public override bool IsValid(ITypeDescriptorContext context, object value)
    {
        if (value is DateOnly)
            return true;
        return base.IsValid(context, value);
    }
}

TypeDescriptor.AddAttributes(typeof(DateOnly), new TypeConverterAttribute(typeof(DateOnlyConverter)));

@tarekgh
Copy link
Member

tarekgh commented May 9, 2022

@thomaslevesque that is right. I believe the ask is to avoid having different users implement the same converters to make their scenarios work.

@tarekgh
Copy link
Member

tarekgh commented May 20, 2022

@0xced I am noticing some differences comparing the proposal to like DateTimeConverter. Any reason for that? you are proposing to have the extra API IsValid (which there is no need to override it) and also you are missing the public constructor and the nullability annotations.

@tarekgh
Copy link
Member

tarekgh commented May 23, 2022

@0xced I have updated the APIs in the proposal. Let's me know if you have any concern.

@tarekgh tarekgh added api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels May 23, 2022
@bartonjs
Copy link
Member

We added System.Half, Int128, and UInt128 to the list to get caught up on primitive-ish types in the TypeConverter space. (We didn't think there was enough of a scenario for NFloat, NInt, or NUInt).

namespace System.ComponentModel
{
    public class DateOnlyConverter : System.ComponentModel.TypeConverter
    {
        public DateOnlyConverter() { }
    }

    public class TimeOnlyConverter : System.ComponentModel.TypeConverter
    {
        public TimeOnlyConverter() { }
    }

    public class HalfConverter : System.ComponentModel.BaseNumberConverter
    {
        public HalfConverter() { }
    }

    public class Int128Converter : System.ComponentModel.BaseNumberConverter
    {
        public Int128Converter() { }
    }

    public class UInt128Converter : System.ComponentModel.BaseNumberConverter
    {
        public UInt128Converter() { }
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 26, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 31, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 2, 2022
@0xced
Copy link
Contributor Author

0xced commented Jun 8, 2022

Thank you for implementing this, Tarek and Jeremy!

@ghost ghost locked as resolved and limited conversation to collaborators Jul 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.ComponentModel
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants