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

Please add support for DateOnly and TimeOnly types #1732

Closed
RobThree opened this issue May 9, 2022 · 5 comments
Closed

Please add support for DateOnly and TimeOnly types #1732

RobThree opened this issue May 9, 2022 · 5 comments
Labels
Area-Parser and Binder bug Something isn't working
Milestone

Comments

@RobThree
Copy link
Contributor

RobThree commented May 9, 2022

See the following example:

using System.CommandLine;
using System.CommandLine.NamingConventionBinder;

internal class Program
{
    public static async Task Main()
    {
        var root = new Command("Root");

        var datecommand1 = new Command("testdate1") { Handler = CommandHandler.Create<DateTime>((date) => Console.WriteLine(date)) };
        datecommand1.AddOption(new Option<DateOnly>(new[] { "--date", "-d" }, "Test date"));

        var datecommand2 = new Command("testdate2") { Handler = CommandHandler.Create<DateOnly>((date) => Console.WriteLine(date)) };
        datecommand2.AddOption(new Option<DateOnly>(new[] { "--date", "-d" }, "Test date"));

        var timecommand1 = new Command("testtime1") { Handler = CommandHandler.Create<DateTime>((time) => Console.WriteLine(time)) };
        timecommand1.AddOption(new Option<TimeOnly>(new[] { "--time", "-t" }, "Test time"));

        var timecommand2 = new Command("testtime2") { Handler = CommandHandler.Create<TimeOnly>((time) => Console.WriteLine(time)) };
        timecommand2.AddOption(new Option<TimeOnly>(new[] { "--time", "-t" }, "Test time"));

        var datetimecommand = new Command("testdatetime") { Handler = CommandHandler.Create<DateTime>((datetime) => Console.WriteLine(datetime)) };
        datetimecommand.AddOption(new Option<DateTime>(new[] { "--datetime", "-dt" }, "Test time"));

        var datetimeoffsetcommand = new Command("testdatetimeoffset") { Handler = CommandHandler.Create<DateTimeOffset>((datetimeoffset) => Console.WriteLine(datetimeoffset)) };
        datetimeoffsetcommand.AddOption(new Option<DateTimeOffset>(new[] { "--datetimeoffset", "-dto" }, "Test time"));

        root.AddCommand(datecommand1);
        root.AddCommand(datecommand2);
        root.AddCommand(timecommand1);
        root.AddCommand(timecommand2);
        root.AddCommand(datetimecommand);
        root.AddCommand(datetimeoffsetcommand);

        var tests = new[]
        {
            new[] { "testdate1", "-d", "2003-02-01" },
            new[] { "testdate2", "-d", "2003-02-01" },
            new[] { "testtime1", "-t", "12:34:56" },
            new[] { "testtime2", "-t", "12:34:56" },
            new[] { "testdatetime", "-dt", "2003-02-01T12:34:56" },
            new[] { "testdatetimeoffset", "-dto", "2003-02-01T12:34:56+02:00" }
        };


        foreach (var t in tests)
        {
            Console.WriteLine($"Test: {string.Join(" ", t)}");
            await root.InvokeAsync(t);
        }
    }
}

When run, this is the output:

est: testdate1 -d 2003-02-01
1-2-2003 00:00:00
Test: testdate2 -d 2003-02-01
1-1-0001
Test: testtime1 -t 12:34:56
9-5-2022 12:34:56
Test: testtime2 -t 12:34:56
00:00
Test: testdatetime -dt 2003-02-01T12:34:56
1-2-2003 12:34:56
Test: testdatetimeoffset -dto 2003-02-01T12:34:56+02:00
1-2-2003 12:34:56 +02:00

The testdate2 and testtime2 "tests" seem to fail; the only difference is that we use DateOnly / TimeOnly for the generic type at the CommandHandler.Create invocations. instead of DateTime

@RobThree
Copy link
Contributor Author

RobThree commented May 9, 2022

Also, counter-intuitively (IMHO), when providing defaults the exact opposite fails (testdate1 and testtime1):

using System.CommandLine;
using System.CommandLine.NamingConventionBinder;

internal class Program
{
    public static async Task Main()
    {
        var root = new Command("Root");

        var datecommand1 = new Command("testdate1") { Handler = CommandHandler.Create<DateTime>((date) => Console.WriteLine(date)) };
        datecommand1.AddOption(new Option<DateOnly>(new[] { "--date", "-d" }, () => DateOnly.Parse("2999-12-31"), "Test date") { IsRequired = false });

        var datecommand2 = new Command("testdate2") { Handler = CommandHandler.Create<DateOnly>((date) => Console.WriteLine(date)) };
        datecommand2.AddOption(new Option<DateOnly>(new[] { "--date", "-d" }, () => DateOnly.Parse("2999-12-31"), "Test date") { IsRequired = false });

        var timecommand1 = new Command("testtime1") { Handler = CommandHandler.Create<DateTime>((time) => Console.WriteLine(time)) };
        timecommand1.AddOption(new Option<TimeOnly>(new[] { "--time", "-t" }, () => TimeOnly.Parse("23:59:59"), "Test time") { IsRequired = false });

        var timecommand2 = new Command("testtime2") { Handler = CommandHandler.Create<TimeOnly>((time) => Console.WriteLine(time)) };
        timecommand2.AddOption(new Option<TimeOnly>(new[] { "--time", "-t" }, () => TimeOnly.Parse("23:59:59"), "Test time") { IsRequired = false });

        var datetimecommand = new Command("testdatetime") { Handler = CommandHandler.Create<DateTime>((datetime) => Console.WriteLine(datetime)) };
        datetimecommand.AddOption(new Option<DateTime>(new[] { "--datetime", "-dt" }, () => DateTime.Parse("2999-12-31T23:59:59"), "Test time"));

        var datetimeoffsetcommand = new Command("testdatetimeoffset") { Handler = CommandHandler.Create<DateTimeOffset>((datetimeoffset) => Console.WriteLine(datetimeoffset)) };
        datetimeoffsetcommand.AddOption(new Option<DateTimeOffset>(new[] { "--datetimeoffset", "-dto" }, () => DateTimeOffset.Parse("2999-12-31T23:59:59Z"), "Test time"));

        root.AddCommand(datecommand1);
        root.AddCommand(datecommand2);
        root.AddCommand(timecommand1);
        root.AddCommand(timecommand2);
        root.AddCommand(datetimecommand);
        root.AddCommand(datetimeoffsetcommand);

        var tests = new[]
        {
            new[] { "testdate1" },
            new[] { "testdate2" },
            new[] { "testtime1" },
            new[] { "testtime2" },
            new[] { "testdatetime" },
            new[] { "testdatetimeoffset" }
        };


        foreach (var t in tests)
        {
            Console.WriteLine($"Test: {string.Join(" ", t)}");
            await root.InvokeAsync(t);
        }
    }
}
Test: testdate1
1-1-0001 00:00:00
Test: testdate2
31-12-2999
Test: testtime1
1-1-0001 00:00:00
Test: testtime2
23:59
Test: testdatetime
31-12-2999 23:59:59
Test: testdatetimeoffset
31-12-2999 23:59:59 +00:00

@elgonzo
Copy link
Contributor

elgonzo commented May 9, 2022

Also, counter-intuitively (IMHO), when providing defaults the exact opposite fails (testdate1 and testtime1):

Not really counter-intuitive in my opnion, considering neither DateOnly, TimeOnly, nor DateTime have conversion operators allowing explicit/implicit conversions between these types (as far as i can tell, according to the documentation).

The behavior i personally find surprising is presented in your first post. Due to the absence of said conversion operators, i would expect testdate1 and testtime1 to fail and not silently ignore the generic type parameters given for the options (DateOnly/TimeOnly).

(testdate2 and testtime2 failing is quite likely due to System.CommandLine not having built-in support for DateOnly/TimeOnly, yet...)

@RobThree
Copy link
Contributor Author

RobThree commented May 9, 2022

Not really counter-intuitive in my opnion

Counter-intuitive in the sense that the cases that failed first when given explicit values now succeed and vice versa.

@elgonzo
Copy link
Contributor

elgonzo commented May 9, 2022

Yeah. In this sense, it's counter-intuitve. I was arguing from the perspective that testdate1 and testtime1 from your first post seemingly working is not being intuitive and expected at all, given the conflict between the options being of DateOnly/TimeOnly type and a handler parameter that is of a type (DateTime) which neither DateOnly nor TimeOnly can be explicitly/implictly converted into.

@jonsequitur
Copy link
Contributor

It's worth pointing out that implicit conversions and support for built-in TypeConverters were removed from the core System.CommandLine. (See #1613 for details). Using CommandHandler.Create (i.e. System.CommandLine.NamingConventionBinder) brings in some additional conversion logic that might confuse discussion of the core parsing behavior, especially given you're specifying a different command handler parameter type than the Option<T> type.

It would be clearer to base the expected versus actual behavior on the GetValueForOption / GetValueForArgument APIs:

var dateOnlyOption = new Option<DateOnly>("-d");
var timeOnlyOption = new Option<TimeOnly>("-t");

var cmd = new RootCommand
{
    dateOnlyOption,
    timeOnlyOption
};

cmd.Parse("-d 2022-05-09").GetValueForOption(dateOnlyOption).Should().Be( /* ??? */ );
cmd.Parse("-t 1:39").GetValueForOption(timeOnlyOption).Should().Be( /* ??? */ );

(testdate2 and testtime2 failing is quite likely due to System.CommandLine not having built-in support for DateOnly/TimeOnly, yet...)

This is likely the core of the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Parser and Binder bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants