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

Tracking Issue for duration_constructors #120301

Open
1 of 3 tasks
djc opened this issue Jan 24, 2024 · 9 comments
Open
1 of 3 tasks

Tracking Issue for duration_constructors #120301

djc opened this issue Jan 24, 2024 · 9 comments
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@djc
Copy link
Contributor

djc commented Jan 24, 2024

Feature gate: #![feature(duration_constructors)]

This is a tracking issue for supporting larger unit sizes for Duration constructors.

Public API

Add the following constructors to Duration:

  • Duration::from_weeks()
  • Duration::from_days()
  • Duration::from_hours()
  • Duration::from_mins()

Steps / History

@djc djc added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 24, 2024
@the8472
Copy link
Member

the8472 commented Jan 24, 2024

Similar things were previously proposed in #47097, #52556
They were intentionally omitted in RFC 1040 because leap seconds complicate things to make time-calculations non-trivial.

Imo it requires some significant discussion to add these. A PR and a tracking issue without context are insufficient.

@djc
Copy link
Contributor Author

djc commented Jan 24, 2024

Thanks for adding this context. I was fairly certain that this would have been discussed before, but searching the issue tracker (for open issues) didn't turn up any existing discussion. I think it's okay to have this discussion here/now?

From RFC 1040:

This proposal does not, at this time, include mechanisms for instantiating a Duration from weeks, days, hours or minutes, because there are caveats to each of those units. In particular, the existence of leap seconds means that it is only possible to properly understand them relative to a particular starting point.

It goes on to quote JodaTime documentation; I'll quote the current version of this:

A period in Joda-Time represents a period of time defined in terms of fields, for example, 3 years 5 months 2 days and 7 hours. This differs from a duration in that it is inexact in terms of milliseconds. A period can only be resolved to an exact number of milliseconds by specifying the instant (including chronology and time zone) it is relative to.

Note that the RFC is from 2015 and the latest discussion in #52556 is from 2018 so I feel like revisiting this decision isn't all that crazy.

Personally I feel this distinction is well-established in the Rust ecosystem at this point, and there's a clear interplay between SystemTime (grounded in real-world time) and Duration (which presents a pure duration). As such, I personally don't feel like leap seconds are a good reason to avoid adding these. I would also argue that both chrono and time are very popular libraries in the ecosystem, and both have made these available seemingly without many ill effects.

IMO each of these constructors have a clear unambiguous meaning in the context of a Duration. If we want, we could of course add some documentation that leap seconds are not considered.

Note: I happen to be the current chrono maintainer, so I have some background in these things.

@Mark-Simulacrum
Copy link
Member

We may want to consider as part of stabilization whether these could "just" (or in addition should be) associated constants. e.g., Duration::WEEK * 5 == Duration::from_weeks(5). We have paired constants for all existing constructor functions: millisecond, second, and nanosecond.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 11, 2024
…Simulacrum

core: add Duration constructors

Add more `Duration` constructors.

Tracking issue: rust-lang#120301.

These match similar convenience constructors available on both `chrono::Duration` and `time::Duration`.

What's the best ordering for these with respect to the existing constructors?
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 11, 2024
Rollup merge of rust-lang#120307 - djc:duration-constructors, r=Mark-Simulacrum

core: add Duration constructors

Add more `Duration` constructors.

Tracking issue: rust-lang#120301.

These match similar convenience constructors available on both `chrono::Duration` and `time::Duration`.

What's the best ordering for these with respect to the existing constructors?
@djc
Copy link
Contributor Author

djc commented Feb 12, 2024

We may want to consider as part of stabilization whether these could "just" (or in addition should be) associated constants. e.g., Duration::WEEK * 5 == Duration::from_weeks(5). We have paired constants for all existing constructor functions: millisecond, second, and nanosecond.

I'm happy to add them, but I'd be disappointed to remove the new constructors in favor of these.

@Victor-N-Suadicani
Copy link
Contributor

Just thought about this and found this issue :)

I prefer the constructors to associated constants.

Have you considered making the constructors take u32 instead of u64, so they don't need to worry about overflow and hence panicking? You lose the option of making a duration longer than u32::MAX * 60 seconds using the from_mins constructor, but that's like 8171 years, so maybe it's okay? 😅

(PS: I think the panic message in from_weeks mistakenly says from_days)

@neumann-paulmann
Copy link

neumann-paulmann commented May 14, 2024

(PS: I think the panic message in from_weeks mistakenly says from_days)

Yup. That looks like a classic case of copy-paste error.

Also

Duration::from_secs(weeks * MINS_PER_HOUR * SECS_PER_MINUTE * HOURS_PER_DAY * DAYS_PER_WEEK)

is rather confusing.
Why not order the constants accordingly to make it simpler to reason about this like of code? E.g.:

Duration::from_secs(SECS_PER_MINUTE * MINS_PER_HOUR * HOURS_PER_DAY * DAYS_PER_WEEK * weeks)

@edwardwc
Copy link

@Mark-Simulacrum @djc I have attempted to add these associated constants in an idiomatic manner: #127700

I would love your feedback!

@eggyal
Copy link
Contributor

eggyal commented Jul 14, 2024

I notice that it isn't currently proposed to add Duration::from_months or Duration::from_years constructors. Why not? Presumably because their durations are variable. Of course, we could document that such constructors are based on a month being exactly 30 days and a year being exactly 365 days, each of exactly 24 hours each of exactly 60 minutes each of exactly 60 seconds... but wouldn't that just be a footgun that users would often misuse and then puzzle over buggy results? Is not the same true of leap seconds and the proposed constructors?

@ChrisDenton
Copy link
Member

We also don't have from_centuries or from_millenia. But these are convenience functions so they're not essential. There's no reason to add them just to round out the API.

It's notable that neither chrono nor time equivalents of Duration are interested in years, which suggests there's no great demand for the API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants