dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.97k stars 4.66k forks source link

[API Proposal]: BCL Type to Represent ISO8601 Durations #72064

Open Foxtrek64 opened 2 years ago

Foxtrek64 commented 2 years ago

Background and motivation

Currently I am integrating with a JSON REST API which returns its time spans formatted as ISO8601 Durations. Parsing these values with the available TimeSpan type is not appropriate due to issues identified in this comment.

API Proposal

This proposal does not add any new APIs. It only modifies existing APIs.

  1. Add constant "o" format specifier for ParseExact() and TryParseExact(). This is in reference to the same value used by DateTime and DateTimeOffset to serialize and deserialize ISO8601 timestamp formats.
  2. Add support for parsing years and months using y, yy, yyyy, yyyyy, and M, MM respectively.
  3. Optionally add support for Parse() and TryParse() if this proves not to be too complicated.

Create a new type, Duration, which is able to adequately represent ISO8601 Duations. I will describe the new type with an interface definition so that implementation details can be described at a later date. The type itself will likely not implement such an interface seeing as ITimeSpan and IDateTime do not exist.

public interface IDuration
{
    // The ISO format does not allow for decimals, so the int type is used.
    // Making the type immutable has its advantages. Modifications can happen through a the with keyword for the record struct type or through AddX() methods.

    int Years { get; init; }
    int Months { get; init; }
    int Days { get; init; }
    int Hours { get; init; }
    int Minutes { get; init; }
    long Seconds { get; init; }
    long Ticks { get; init; }

    // A default, zeroed instance of a duration.
    public static Duration Zero { get; }

    public Duration Parse(string iso8601duration);
    public bool TryParse(string iso8601duration, out Duration duration);

    // Default parameterless constructor, record constructor for properties, and cloning constructor should be all that's needed. They will not be described here.

    // As the duration struct is immutable, these helper methods exist to perform mathematical operations.
    // Static variations accepting two Durations as well as the corresponding operators should be implemented.
    public Duration Add(Duration other);
    public Duration Subtract(Duration other);

    // Returns an equivalent but not necessarily equal duration based on the overall ticks value.
    // For instance, a duration with a time of 25 hours would be normalized to a time of 1 day and 1 hour.
    // The old duration and new duration would be equivalent but would not be equal.
    // This method functions identically to Period.Normalize() from NodaTime.
    public Duration Normalize();

    // A static helper which returns a duration created from the corresponding number of units.
    // A FromX() method should be created for each of the various 
    public static Duration FromX(int x);

    // Sometimes this conversion can be useful. Add an explicit conversion operator as well.
    public static Duration FromTimeSpan(TimeSpan timeSpan);

    // Equals() should compare two durations based on their ticks, not their actual values. If actual value comparison is important, the `is` keyword can be used.

    // The ToString() method should return the following format: "P#Y#M#DT#H#M#S", where the # represents a particular integer value.
}

I think I got everything, but please do let me know if there is anything I should add to the proposal.

I do have concerns over the name Duration. I feel it makes the most sense from the perspective of representing an ISO8601 Duration, however this name conflicts with the NodaTime library in a way that may cause a rather painful transition for developers seeking to use the BCL type rather than NodaTime.Period. NodaTime.Duration represents a different concept, and so having references to both System.Duration and NodaTime.Duration may create ambiguity and headaches.

Normally I would not take the names used by third party libraries into consideration, however with NodaTime being the most popular time library, I feel that willfully ignoring its existence and forging ahead would be foolhardy and lead to a lot of very annoyed developers. As such, I am seeking suggestions for alternative names to move away from both Period and Duration, even if such a name is simply ISO8601Duration.

API Usage

string duration = "P3Y6M4DT99H30M5S";
var duration = Duration.Parse(duration); // 3 years, 6 months, 0 Weeks, 4 days, 99 hours, 30 minutes, 5 seconds.
var normalized = duration.Normalize(); // 3 years, 6 months, 0 weeks, 8 days, 3 hours, 30 minutes, 5 seconds

Alternative Designs

Currently, a sort-of workaround is to use XmlConvert.ToTimeSpan(string duration) and XmlConvert.ToString(TimeSpan duration), however this lives in the System.Xml namespace and usually makes little sense to add, especially when working with JSON. Additionally, as pointed out by @tthiery, this does not function with parsing months or years, so it's not to be used as a drop-in replacement for this proposed type.

Risks

No response

Honorable Mentions

28942 is specifically for JSON parsing and these issues will likely overlap. Their issue is specifically DateTime and DateTimeOffset.

Edit: Restructured proposal to create new type rather than update TimeSpan.

dotnet-issue-labeler[bot] commented 2 years ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

ghost commented 2 years ago

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

Issue Details
### Background and motivation Currently I am integrating with a JSON REST API which returns its timespans formatted as [ISO8601 Durations](https://en.wikipedia.org/wiki/ISO_8601#Durations). Attempting to parse these values with `TimeSpan.Parse()` is currently not available. Parsing with `TimeSpan.ParseExact()` may work with a custom formatting string, but I have not had any luck and currently, no support for parsing groups larger than days exists. ### API Proposal This proposal does not add any new APIs. It only modifies existing APIs. 1. Add constant "o" format specifier for `ParseExact()` and `TryParseExact()`. This is in reference to the same value used by DateTime and DateTimeOffset to serialize and deserialize ISO8601 timestamp formats. 2. Add support for parsing years and months using `y`, `yy`, `yyyy`, `yyyyy`, and `M`, `MM` respectively. 3. Optionally add support for `Parse()` and `TryParse()` if this proves not to be too complicated. ### API Usage ```csharp string duration = "P3Y6M4DT12H30M5S"; var timespan = TimeSpan.ParseExact(duration, "o", null); // 3 years, 6 months, 4 days, 12 hours, 30 minutes, 5 seconds. ``` ### Alternative Designs Currently a viable workaround is to use `XmlConvert.ToTimeSpan(string duration)` and `XmlConvert.ToString(TimeSpan duration)`, however this lives in the `System.Xml` namespace and usually makes little sense to add, especially when working with JSON. ### Risks _No response_ ### Honorable Mentions #28942 is specifically for JSON parsing and these issues will likely overlap. Their issue is specifically DateTime and DateTimeOffset.
Author: Foxtrek64
Assignees: -
Labels: `api-suggestion`, `area-System.Runtime`, `untriaged`
Milestone: -
Clockwork-Muse commented 2 years ago

Being able to parse into a TimeSpan isn't going to solve any problems, it's immediately going to give you a new one: For any field larger than "Day", durations are only relative. "Month", for instance, may be equivalent to 28, 29, 30, or 31 days. You cannot reliably translate this to a TimeSpan and then add it to a DateTime/DateTimeOffset, you're going to be dealing with incorrect values.

We'd need to create a type that's the equivalent of the Period class from NodaTime/JSR310 to be able to actually handle this.


As a side note, if you're actually dealing with date/time problems like this, you're probably better advised to just use NodaTime anyways, as there are a number of shortcomings in the BCL that it addresses (although not as many as there used to be).


Actually, technically "Day" is sometimes relative too, about once every 18 months, due to Leap Seconds. Although most APIs I'm aware of ignore them anyways.

tannergooding commented 2 years ago

CC. @tarekgh, is this something we should look at providing?

Foxtrek64 commented 2 years ago

With the shortcomings @Clockwork-Muse brought up, if you guys think modifying this proposal to instead create a new Duration type is a better answer, I can always update the proposal. That could potentially resolve some of the issues, but of course we run into that training issue of getting people to move from TimeSpan to Duration, same as getting people to move from DateTime to DateTimeOffset.

tarekgh commented 2 years ago

I agree with @Clockwork-Muse, cannot parse duration format to a TimeSpan. We may consider supporting duration by introducing a new type. We just want to see a demand for that, and we can consider it.

Foxtrek64 commented 2 years ago

I agree with @Clockwork-Muse, cannot parse duration format to a TimeSpan. We may consider supporting duration by introducing a new type. We just want to see a demand for that, and we can consider it.

I'll go ahead and update the proposal to recommend the creation of a new type. This will help us better gauge demand.

tthiery commented 1 year ago

Please remove System.Xml.XmlConvert.ToTimeSpan as a viable design. It does only support normalized to days which is highly inaccurate when working with months or years. The parsed timespan is generally not accepted as a suitable (see #28862).

@eiriktsarpalis The community filed already an API Proposal suitable for the issue #28862 😀.

@Foxtrek64 thanks ;) saved me two hours

oising commented 1 year ago

I agree with @Clockwork-Muse, cannot parse duration format to a TimeSpan. We may consider supporting duration by introducing a new type. We just want to see a demand for that, and we can consider it.

Is there anything particularly wrong about forcing the caller to provide an anchor/base datetime to give a context for parsing the duration into a TimeSpan? This would at least force developers to acknowledge the ambiguities, while also giving them what they ask for. In any other platforms, the anchor/base is implicit, so making it explicit here seems logical [to me.]

i.e. TimeSpan.Parse(string is8601duration, DateTimeOffset base)

In short, people are going to use XmlConvert and be forced to handle months/years themselves, and invariably make mistakes. Let's help people fall into the pit of success.

WhatzGames commented 1 year ago

Should it be considered to add some new new operators for working with TimeOnly and DateOnly?

public static TimeOnly operator +(TimeOnly left, Duration right);
public static TimeOnly operator -(TimeOnly left, Duration right);

public static DateOnly operator +(DateOnly left, Duration right);
public static DateOnly operator -(DateOnly left, Duration right);

public static Duration operator -(DateOnly left, DateOnly right);
public static Duration operator -(TimeOnly left, TimeOnly right);

These were the ones that would make sense to me. Calculating differences between Times or Dates. Adding a Duration to a DateOnly would only add everything from Days, Weeks, Months and Years. The TimeOnly then only adds the time components and would overflow back to zero

mstgms commented 4 months ago

Is there any progress? We also need can parse duration values which sents from clients in jsons like "PT7H0.301S" on our Financial APIs. +@muazcoban

tarekgh commented 4 months ago

We're currently occupied with higher-priority tasks. We'll address this at a later time. Thank you!

Clockwork-Muse commented 4 months ago

@mstgms - especially if you're dealing with many of the scenarios where date-based durations make sense, you may find the overall API from NodaTime more ergonomic for many tasks, which also includes a Period type.

julealgon commented 3 months ago

When/if this is implemented, will the framework start adding overloads to timeout-related methods that take a Duration instead of a TimeSpan? Any other areas in the framework where having this type would be better than a TimeSpan currently?

Would this also potentially deprecate TimeSpan over time? Is there any specific reason someone would not use a Duration instead of a TimeSpan in their domains?

tarekgh commented 3 months ago

When/if this is implemented, will the framework start adding overloads to timeout-related methods that take a Duration instead of a TimeSpan? Any other areas in the framework where having this type would be better than a TimeSpan currently?

This depends on if it makes sense to have any API accept duration. That needs to look at case by case to decide that.

Would this also potentially deprecate TimeSpan over time? Is there any specific reason someone would not use a Duration instead of a TimeSpan in their domains?

I don't think we need to deprecate TimeSpan at all. TimeSpan and Duration are needed for different purposes. You may look at the blog https://thecontentauthority.com/blog/span-vs-duration describing the difference between the time span and the duration.