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]: BCL Type to Represent ISO8601 Durations #72064

Open
Foxtrek64 opened this issue Jul 12, 2022 · 15 comments
Open

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

Foxtrek64 opened this issue Jul 12, 2022 · 15 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.DateTime
Milestone

Comments

@Foxtrek64
Copy link
Contributor

Foxtrek64 commented Jul 12, 2022

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.

@Foxtrek64 Foxtrek64 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jul 12, 2022
@dotnet-issue-labeler
Copy link

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 ghost added the untriaged New issue has not been triaged by the area owner label Jul 12, 2022
@ghost
Copy link

ghost commented Jul 13, 2022

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. 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

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
Copy link
Contributor

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 tannergooding added this to the Future milestone Jul 15, 2022
@tannergooding tannergooding added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed untriaged New issue has not been triaged by the area owner labels Jul 15, 2022
@tannergooding
Copy link
Member

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

@Foxtrek64
Copy link
Contributor Author

Foxtrek64 commented Aug 26, 2022

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
Copy link
Member

tarekgh commented Aug 26, 2022

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
Copy link
Contributor Author

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.

@Foxtrek64 Foxtrek64 changed the title [API Proposal]: TimeSpan - Parse ISO8601 Durations [API Proposal]: BCL Type to Represent ISO8601 Durations Aug 26, 2022
@tthiery
Copy link

tthiery commented Oct 10, 2022

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
Copy link

oising commented May 15, 2023

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.

@tarekgh tarekgh removed the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label May 15, 2023
@WhatzGames
Copy link

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
Copy link

mstgms commented May 7, 2024

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
Copy link
Member

tarekgh commented May 7, 2024

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

@Clockwork-Muse
Copy link
Contributor

@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
Copy link

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
Copy link
Member

tarekgh commented Jun 21, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.DateTime
Projects
None yet
Development

No branches or pull requests