RFC: Bring some of the Rubedo extensions for Chrono into Chrono

[danwilliams]

@djc @pitdicker I have a number of pieces of functionality to extend Chrono, which are available under my Rubedo crate, with there being a specific module for the Chrono extensions.

I would like to bring some of that functionality into Chrono, to benefit a wider audience, but I am not sure how much of it is appropriate and would gain approval, so I am raising this RFC ticket to get feedback on that before I spend time bringing anything across into Chrono PRs.

Here are some initial thoughts, although I would welcome perusal of the full page linked to:

• I see a lot of value in the "full" functions for Duration, which provide constructors and getters for using the full range of nanoseconds and microseconds that a Duration can store. Due to the limitations of the native Chrono functions in this regard, I believe these functions would be useful to have in the main library, and they could even replace the current functions (as they extend the range) - although this would be a breaking change in my view, as they accept/return i128 instead of i64. So perhaps just adding my functions would be sufficient here.
• I think it would be useful to many people to bring in the additional functions I have for NaiveDate, as they are common operations that have been very helpful in my various project codebases.
• The other functionality may not be as suitable for a general audience - Duration.humanize() is useful but specific, and I doubt you will want the Months extensions as I know you have a CalendarDuration on the way 🙂
• I also have a range of constants, which I find useful, but which are not directly beneficial to Chrono because there are other ways of getting the same information. BUT as some of the Chrono constants are not public, it would be great to have permission to revise the available constants and make a sensible and useful range public to people.

I don't know how these suggestions fit in with your plans for v0.5, but I think they can all target v0.4.x as they are fully compatible. I am of course open to adjusting the approach, naming, scope, etc. of any of these functions if you want them brought in, and would also submit them according to Chrono's coding standards as I have done with my other PRs.

Let me know!

[pitdicker]

I'll need a couple of comments to form an opinion.

List of the constants and methods in your Rubedo crate:

impl Duration {
    const MAX_NANOSECONDS: i64 = 9_223_372_036_854_775_807i64;
    const MAX_NANOSECONDS_FULL: i128 = 9_223_372_036_854_775_807_000_000i128;
    const MAX_MICROSECONDS: i64 = 9_223_372_036_854_775_807i64;
    const MAX_MICROSECONDS_FULL: i128 = 9_223_372_036_854_775_807_000i128;
    const MAX_MILLISECONDS: i64 = 9_223_372_036_854_775_807i64;
    const MAX_SECONDS: i64 = 9_223_372_036_854_775i64;
    const MAX_MINUTES: i64 = 153_722_867_280_912i64;
    const MAX_HOURS: i64 = 2_562_047_788_015i64;
    const MAX_DAYS: i64 = 106_751_991_167i64;
    const MAX_WEEKS: i64 = 15_250_284_452i64;
    const MIN_NANOSECONDS: i64 = -9_223_372_036_854_775_808i64;
    const MIN_NANOSECONDS_FULL: i128 = -9_223_372_036_854_775_807_000_000i128;
    const MIN_MICROSECONDS: i64 = -9_223_372_036_854_775_808i64;
    const MIN_MICROSECONDS_FULL: i128 = -9_223_372_036_854_775_807_000i128;
    const MIN_MILLISECONDS: i64 = -9_223_372_036_854_775_807i64;
    const MIN_SECONDS: i64 = -9_223_372_036_854_775i64;
    const MIN_MINUTES: i64 = -153_722_867_280_912i64;
    const MIN_HOURS: i64 = -2_562_047_788_015i64;
    const MIN_DAYS: i64 = -106_751_991_167i64;
    const MIN_WEEKS: i64 = -15_250_284_452i64;
    const UNITS: [(i64, &'static str); 7] = _;

    fn nanoseconds_full(nanoseconds: i128) -> Option<Duration>;
    fn microseconds_full(microseconds: i128) -> Option<Duration>;
    fn num_nanoseconds_full(&self) -> i128;
    fn num_microseconds_full(&self) -> i128;
}

impl Months {
    const MAX_MONTHS: u32 = 4_294_967_295u32;
    const MAX_YEARS: u32 = 357_913_941u32;

    fn months(months: u32) -> Self;
    fn years(years: u32) -> Option<Months>;
    fn num_months(&self) -> u32;
    fn num_years(&self) -> u32;
}

impl NaiveDate {
    const MAX_YEAR: i32 = 262_142i32;
    const MIN_YEAR: i32 = -262_143i32;

    fn today() -> NaiveDate;
    fn days_in_month(&self) -> u32;
    fn days_in_month_opt(year: i32, month: u32) -> Option<u32>;
    fn days_in_year(&self) -> u32;
    fn days_in_year_opt(year: i32) -> Option<u32>;
    fn is_leap_year(&self) -> bool;
    fn is_leap_year_opt(year: i32) -> Option<bool>;
    fn start_of_month(&self) -> NaiveDate;
    fn start_of_month_opt(year: i32, month: u32) -> Option<NaiveDate>;
    fn end_of_month(&self) -> NaiveDate;
    fn end_of_month_opt(year: i32, month: u32) -> Option<NaiveDate>;
    fn start_of_year(&self) -> NaiveDate;
    fn start_of_year_opt(year: i32) -> Option<NaiveDate>;
    fn end_of_year(&self) -> NaiveDate;
    fn end_of_year_opt(year: i32) -> Option<NaiveDate>;
}

[pitdicker]

Months

This partly matches your work in #1373. With Months::new and Months::as_u32 we have the basic functionality to create a value and get the number back out again.

Like @djc I am not a fan of Months as part of the API. I suppose it fits in some coding styles. But if you go with the principle that the API of chrono should steer users to a solution that avoids subtle mistakes... it feels like this was not the right direction.

You would mainly want to add years and num_years to the type. I don't think it makes what we have any worse.

[pitdicker]

Duration

• I see a lot of value in the "full" functions for Duration, which provide constructors and getters for using the full range of nanoseconds and microseconds that a Duration can store. Due to the limitations of the native Chrono functions in this regard, I believe these functions would be useful to have in the main library, and they could even replace the current functions (as they extend the range) - although this would be a breaking change in my view, as they accept/return i128 instead of i64. So perhaps just adding my functions would be sufficient here.

The API of chrono is, in Rust terms, ancient 😆. These methods pre-date the stable i128 integers by ca. 4 years. I'm not sure, are i128 integers well-supported and reasonably performant on all platforms?

We could bikeshed the names, but I see no harm in having them. Also not much benefit to be honest, the PR should come with some good rationale.

What I like to see more is conversion to/from floating point values #1365.

[pitdicker]

NaiveDate

is_leap_year is supported since 0.4.30: https://docs.rs/chrono/0.4.33/chrono/naive/struct.NaiveDate.html#method.leap_year.

days_in_month, days_in_year, start_of_month, end_of_month, start_of_year, end_of_year: there is some overlap with this PR: #893.
Having more methods to work on dates is a something we have more requests for, like #29 and #69. I certainly recognise these methods as something I would like to use.

However once you start adding convenience methods like this, where do you draw the line? We currently have the line at little to none, and that is easy to defend.

I wonder if it would be good to have an extension trait for everything-and-the-kitchen-sink convenience methods, with no stability guarantees when it comes to extending the trait.

[pitdicker]

@danwilliams Some general comments. All just my personal thoughts, @djc is the primary maintainer.

I only got involved around March/April, then dropped out for a couple of months, and am just getting back again.
@jtmoon79 has been around a little longer, and he mostly helps out with tests and reviews.

Basically the API of chrono is large and pre-dates Rust 1.0 and current conventions. The hope is to reduce the API surface, make methods less panic-prone, use Result types and some nicer method names. But the chrono 0.5 branch is making little progress.

I have a good number of open PRs that tackle tricky issues, mostly against the branch for 0.4. They represent a lot of review-work which mostly comes on the shoulders of @djc, who already does an amazing amount of work. Not sure how to make good progress there to be honest.

We got a lot done last year, but there is also a lot left to do for a good minimal 0.5 release with a nicer API.

Chrono could use some more help, and you seem to be willing to contribute. Do you want to dive into some of the open PRs or issues?

O, and have you found our discord? Linked as a badge on the readme.

[jtmoon79]

I really like this crate @danwilliams !

Here are my recommended "takes" to copy from rubedo to chrono, listed by module.

---

rubedo::chrono constants

First, regarding new constants, bringing in constants related to newly introduced API functions is low-effort, low-risk, low maintenance, and just darn useful for users.

---

rubedo::chrono::DurationExt features

This is a really excellent point

fn microseconds_full(microseconds: i128) -> Option<Self>
Make a new Duration with the given number of microseconds.
This function is necessary because although the Duration struct stores its value internally as seconds and microseconds, the microseconds() method only accepts the number of microseconds as an i64, which is not large enough to express the full range of microseconds that can be stored by a Duration instance. This function therefore accepts the number of microseconds as an i128, which is large enough to express the full range of microseconds that can be stored, and creates a Duration instance appropriately.

I think chrono should be updated to include this. Two general paths I see:

1. major version breaking change for Duration and microseconds,
   • replacing current pub const fn microseconds(microseconds: i64) -> Duration with pub const fn microseconds(microseconds: i128) -> Duration. Same for the nanoseconds functionality.
   • change the current chrono pub struct Duration internal secs: i64 to secs: i128
2. no breaking change: add the function microseconds_full and nanoseconds_full to exist side-by-side with current chrono API

So from rubedo::chrono::DurationExt I would take

impl Duration {
    const MAX_NANOSECONDS: i64 = 9_223_372_036_854_775_807i64;
    const MAX_NANOSECONDS_FULL: i128 = 9_223_372_036_854_775_807_000_000i128;
    const MAX_MICROSECONDS: i64 = 9_223_372_036_854_775_807i64;
    const MAX_MICROSECONDS_FULL: i128 = 9_223_372_036_854_775_807_000i128;
    const MAX_MILLISECONDS: i64 = 9_223_372_036_854_775_807i64;
    const MAX_SECONDS: i64 = 9_223_372_036_854_775i64;
    const MAX_MINUTES: i64 = 153_722_867_280_912i64;
    const MAX_HOURS: i64 = 2_562_047_788_015i64;
    const MAX_DAYS: i64 = 106_751_991_167i64;
    const MAX_WEEKS: i64 = 15_250_284_452i64;
    const MIN_NANOSECONDS: i64 = -9_223_372_036_854_775_808i64;
    const MIN_NANOSECONDS_FULL: i128 = -9_223_372_036_854_775_807_000_000i128;
    const MIN_MICROSECONDS: i64 = -9_223_372_036_854_775_808i64;
    const MIN_MICROSECONDS_FULL: i128 = -9_223_372_036_854_775_807_000i128;
    const MIN_MILLISECONDS: i64 = -9_223_372_036_854_775_807i64;
    const MIN_SECONDS: i64 = -9_223_372_036_854_775i64;
    const MIN_MINUTES: i64 = -153_722_867_280_912i64;
    const MIN_HOURS: i64 = -2_562_047_788_015i64;
    const MIN_DAYS: i64 = -106_751_991_167i64;
    const MIN_WEEKS: i64 = -15_250_284_452i64;

    fn nanoseconds_full(nanoseconds: i128) -> Option<Duration>;
    fn microseconds_full(microseconds: i128) -> Option<Duration>;
    fn num_nanoseconds_full(&self) -> i128;
    fn num_microseconds_full(&self) -> i128;

Not included was humanize; it feels a little "squishy".

---

rubedo::chrono::MonthsExt features

Seconding @pitdicker point

Like @djc I am not a fan of Months as part of the API. I suppose it fits in some coding styles. But if you go with the principle that the API of chrono should steer users to a solution that avoids subtle mistakes...

What really needs to happen is another papal bull wherein the Pope can introduce a Pull Request to fix the Gregorian Calendar 😉

---

rubedo::chrono::NaiveDateExt features

I really like days_in_month, and days_in_year, and is_leap_year.

While @pitdicker has a reasonable precaution:

However once you start adding convenience methods like this, where do you draw the line? We currently have the line at little to none, and that is easy to defend.

But those three specific methods are very useful because of the arbitrary oddities of the Gregorian Calendar and the leap day convention. I have a hunch these functions are written from scratch fairly often.

The other X_in_Y functions are also helpful. My preference is to include them but my justification is weaker.

I'm not sure about today as it introduces the contextual oddity of which timezone the system reports... but it is convenient. 🤔

So from rubedo::chrono::NaiveDateExt I recommend minimally

impl NaiveDate {
    const MAX_YEAR: i32 = 262_142i32;
    const MIN_YEAR: i32 = -262_143i32;

    fn days_in_month(&self) -> u32;
    fn days_in_month_opt(year: i32, month: u32) -> Option<u32>;
    fn days_in_year(&self) -> u32;
    fn days_in_year_opt(year: i32) -> Option<u32>;
    fn is_leap_year(&self) -> bool;
    fn is_leap_year_opt(year: i32) -> Option<bool>;
}

but personally I also like

impl NaiveDate {
    fn start_of_month(&self) -> NaiveDate;
    fn start_of_month_opt(year: i32, month: u32) -> Option<NaiveDate>;
    fn end_of_month(&self) -> NaiveDate;
    fn end_of_month_opt(year: i32, month: u32) -> Option<NaiveDate>;
    fn start_of_year(&self) -> NaiveDate;
    fn start_of_year_opt(year: i32) -> Option<NaiveDate>;
    fn end_of_year(&self) -> NaiveDate;
    fn end_of_year_opt(year: i32) -> Option<NaiveDate>;
}

Not sure about today.

[pitdicker]

First, regarding new constants, bringing in constants related to newly introduced API functions is low-effort, low-risk, low maintenance, and just darn useful for users.

I am not so sure about the Duration constants. The range of the type is almost 300 million years with nanosecond accuracy. Do we expect users to bump into its limitations so often it needs we need to add 20 constants describing its range?

@danwilliams I guess you had to add these constants because before chrono 0.4.32 the initializers would panic on out of bound values. Do you still see a need for them now that we have alternatives that return an option?

Note that we already have Duration::max_value, and #1337 to make the methods on Duration const. So in the case you still need these constants you could write Duration::max_value().num_seconds() instead of MAX_SECONDS.

What really needs to happen is another papal bull wherein the Pope can introduce a Pull Request to fix the Gregorian Calendar 😉

🤣 We would just get different trade-offs though, and still have to support the existing system. No thanks.

[pitdicker]

I'm not sure about today as it introduces the contextual oddity of which timezone the system reports... but it is convenient. 🤔

Forgot about this one. We used to have Local::today, which returned the not entirely sensible Date<Tz> type and was therefore deprecated. A method that returns a NaiveDate seems convenient.

On the other hand it is just Local::now().date_naive(). That does not seem excessive, and is more explicit about local versus UTC. Maybe all we should do is use it a bit more in the doc examples?

[pitdicker]

NaiveDate::end_of_month

Last summer I was writing a fast, compactly encoded alternative to chrono_tz (not finished sadly). And 'last day of the month' or 'last x weekday of the month` is needed for a lot of transition date rules. I wanted to have a reliable, fast way to get that.

The current methods to get the last day of the month are:

• Look up if the year is a leap year, look up the number of days in the month, and convert year-month-day to a NaiveDate (looking up the year flags again).
• Add one to the month, take mod 12, adjust the year if the month was december, convert year-month-day to a NaiveDate, and calculate the preceding day (which may internally need to recreate the NaiveDate if it wraps a year boundary).

The last solution is recommended in our documentation. Both solutions are not especially efficient, and the last solution has the disadvantage that it can not return NaiveDate::MAX as a result.

I found a neat trick that seemed to fit in the spirit of chrono's design.

To convert year-month-day to a NaiveDate chrono uses a lookup table with 10-bit indexes: 1 bit for leap years, 4 bits for the month, 5 bits for the day of the month. With just 31 values for the day of the month we have room for one more value in the lookup table to encode the last day of the month. That makes creating a NaiveDate for the last day of the month as fast and simple as any other day.

So end_of_month has the technical advantages that it can be less error-prone than writing by hand, can be much faster, and is able to return all dates including NaiveDate::MAX.

Edit: a fast solution would also be helpful when doing calculations with a CalendarDuration. I found the commit with this idea in one of my branches for that: a1cbdf1.

[djc]

This discussion is a little unstructured, so here's a proposed structure: everyone who wants to participate, please pick your top 5 added methods/constants (one constant or method per item!) from the Rubedo crate and list them, in most-to-least important order, with a quick explanation for how/where you'd use it.

[pitdicker]

I was not even at the point yet to have a wishlist 😄. Basically just thinking out loud.

Three seem useful to me: NaiveDate::end_of_month, NaiveDate::MAX_YEAR and NaiveDate::MIN_YEAR.
A few others are a maybe.

---

As a list:

• NaiveDate::end_of_month(year: i32, month: u32) -> Option<NaiveDate>;

  This one seems most useful to me. It can also be used as a building block for the other suggested methods, which then become less compelling to add:

  • NaiveDate::end_of_year is NaiveDate::end_of_month(year, 12)
  • NaiveDate::days_in_month is NaiveDate::end_of_month(year, month)?.day()
  • NaiveDate::days_in_year is NaiveDate::end_of_month(year, 12)?.ordinal() or if NaiveDate::ymd_opt(year, 1, 1)?.leap_year() { 366 } else { 365 }

• impl NaiveDate {
      const MAX_YEAR: i32 = 262_142i32;
      const MIN_YEAR: i32 = -262_143i32;
  }

  Seem like a useful addition. We have no alternative to get those constants in const context yet.

• impl Duration {
      fn nanoseconds_full(nanoseconds: i128) -> Option<Duration>;
      fn microseconds_full(microseconds: i128) -> Option<Duration>;
      fn num_nanoseconds_full(&self) -> i128;
      fn num_microseconds_full(&self) -> i128;
  }

  I see no harm in having these. Also not much benefit to be honest, the PR should come with some good rationale.

• NaiveDate::today()

  Could be useful. I prefer to instead mention Local::now().date_naive() a bit more often in the documentation.

• impl Months {
      fn years(years: u32) -> Option<Months>;
      fn num_years(&self) -> u32;
  }

  Not a fan of the type, but I also see no harm in adding these methods.

[djc]

You didn't do use cases, but here are some thoughts:

• NaiveDate::end_of_month() does seem useful, it seems like a good sign that it can help express some of the others
• Instead of adding NaiveDate::MAX_YEAR, I think I'd prefer adding a const method that can be accessed via NaiveDate::MAX
• I don't love the _full() methods as the semantics seems not intuitively obvious to me (maybe because of the name)
• NaiveDate::today() seems like a footgun to me because it has an implicit reliance on the local timezone. I think Local::now().date_naive() seems better.
• I don't want to add more API surface to Months, as discussed before.

[pitdicker]

Then we seem to have an answer for @danwilliams.

• I see a lot of value in the "full" functions for Duration, which provide constructors and getters for using the full range of nanoseconds and microseconds that a Duration can store.

Not entirely convinced yet, they would need a use case. It may just be the name; maybe use _i128 as function names?

• I think it would be useful to many people to bring in the additional functions I have for NaiveDate, as they are common operations that have been very helpful in my various project codebases.

NaiveDate::end_of_month() would be nice: it can function as a building block, the workarounds are multiple lines, and because it can have better performance.

Other methods would need an explanation why alternatives using the currently available methods are not nice enough.

• The other functionality may not be as suitable for a general audience - Duration.humanize() is useful but specific, and I doubt you will want the Months extensions as I know you have a CalendarDuration on the way 🙂

Agreed.

• I also have a range of constants, which I find useful, but which are not directly beneficial to Chrono because there are other ways of getting the same information. BUT as some of the Chrono constants are not public, it would be great to have permission to revise the available constants and make a sensible and useful range public to people.

Hopefully most would not be needed when a couple more methods become const.

[danwilliams]

@pitdicker @jtmoon79 @djc thank you all very much for your time and consideration! 🙂 I appreciate the effort you have gone to in order to review and assess - and each of your suggestions and points of view are useful and insightful. To be honest there is more support and positivity than I had expected, so I am very pleased that there will be some things of use I can bring across in order to benefit the main crate.

I waited a day before replying, to give time for the discussion to take place, and to ensure that all three of you had chance to respond. Please find below my thoughts on each area discussed.

Duration

@pitdicker: are i128 integers well-supported and reasonably performant on all platforms?

As far as I am aware, yes 🙂 Although the performance will not be as good as 64-bit, 128-bit is necessary to represent the full range that a Duration can store.

@pitdicker: We could bikeshed the names, but I see no harm in having them.

I'm very open to direction on the names - I picked what I thought was best, but, that was for my own crate. They can be called anything.

@pitdicker: ...the PR should come with some good rationale.

The rationale is basically this: when you create a Duration, the internal storage approach chosen (i.e. the combination of i64 and i32), along with the limits put in place, means that it is possible to have a total range which, when expressed as nanoseconds or microseconds, is greater than the maximum range expressible in an i64.

Now... I personally don't think Durations should be signed, and I also don't necessarily agree with the limits, BUT these were not my decisions 😄 So with the current implementation, it remains the fact that you cannot easily create or query a Duration with more than i64::MAX nanoseconds.

There is a predictable counterpoint that occasionally comes up with discussions like this, which is "...but why would anyone want to do that?". I see that as a faulty question: I am not concerned with "why" people will do X, as there are always reasons we can't think of right away, but rather, how they can use the functionality to the full extent permitted. Ergo: as the Duration type allows greater than 64 bits of nanosecond storage, it should make available functions for using the full range of this storage. Or, it should decrease the allowed storage to be within the expressible range.

Nevertheless, I can, and will, provide a use case: I have some code that works with metrics and statistics. It is quite often the case that such functionality will increment a count and then divide it to obtain an average. So, you may have let sum = Duration::nanoseconds(x); and then add to this value every time there is an update. Now let's think about the API functionality available to achieve this task: the natural thing would be to write a check to ensure that sum is within the bounds of MAX, i.e. Duration::max_value(). Good, that's easy. But now, how do we get the full value out? Oh - we can't. Or, how do we perform standard arithmetic where we may want to assign a value of x to a new Duration, where x is the total nanoseconds... we can't, if it overflows. So this limits the range of the Duration in a way that is uncheckable through the provided interface: instead, you have to discover more about the internal workings of the Duration, and like implement your own check for i64::MAX, which then is brittle and does not rely upon the central library provisions. It's just not great. Finally, yes, it would be possible to for instance update an average instead of a sum, but this is not generally the best approach because it's desirable to limit FP operations plus accuracy will get lost over time. Hence metrics functionality tends to sum as much as it can, as that is quick and static, and then perform expensive FP when required.

Ultimately, my use case doesn't matter - it is near to hand, and helps illustrate, but the main point is that if something is provided, then full operation for the thing should also be provided.

@jtmoon79: This is a really excellent point

I can see that you immediately understand the problem! 😄 Which of the two options you listed did you prefer? I felt adding my methods has less impact, but it's not ideal - ideal would be one unified approach. Perhaps my methods could be added in 0.4.x and then unified in 0.5 where we can make wider changes?

@djc: I don't love the _full() methods as the semantics seems not intuitively obvious to me (maybe because of the name)

I don't disagree, and I would very much appreciate your thoughts around naming here. It may well depend on the choice of whether to replace the existing functions or not.

@pitdicker: ...maybe use _i128 as function names?

Personally, although I am not totally against that (and would just like to see the functionality available!), I feel that an _i128 suffix does not make it clear why it's an i128, and is also too specific to the type. I don't find it semantic. The purpose is, "let me use the full range", and is necessary because the standard functions don't allow the full range. I am sure my names can be better though! 🙂

@djc: NaiveDate::today() seems like a footgun to me because it has an implicit reliance on the local timezone. I think Local::now().date_naive() seems better.

I think this has been misunderstood - to me, it does not make sense to construct a NaiveDate using a local timezone: Utc would be the sensible default here. Therefore, that is why my code does Utc::now().date_naive(), i.e. Utc rather than Local. There are definitely things to be aware of, but nothing that can't be safeguarded with appropriate documentation (which prompts me that my own documentation for the function does not mention the timezone... ooops! 😊).

NaiveDate

@pitdicker: is_leap_year is supported since 0.4.30:

Interesting - I did not know that. I think my function predates it, and I'd not noticed leap_year() being added to perform the same role. Is there any reason behind the naming not following the more-common is_ prefix? Just curious.

@pitdicker: However once you start adding convenience methods like this, where do you draw the line? We currently have the line at little to none, and that is easy to defend.

Personally I like as many convenience functions to be available as will be useful to people interacting, whilst avoiding clutter. The easier people can achieve the task, they more they will like the library. This is a general statement, and not specific to Chrono.

@pitdicker: I wonder if it would be good to have an extension trait for everything-and-the-kitchen-sink convenience methods

Your idea of an extension trait is interesting - I would be happy to add some things in that way, if desirable, BUT I wonder is that convolution for the sake of a faulty ideal? Does it make it harder for people to find and use what they need? After all, the Rust compiler only builds in what's used, so presenting additional options to people is a question of documentation and discoverability, rather than performance.

@jtmoon79: I really like days_in_month, and days_in_year, and is_leap_year.

...those three specific methods are very useful because of the arbitrary oddities of the Gregorian Calendar and the leap day convention. I have a hunch these functions are written from scratch fairly often.

Agreed! Exactly why I added them to a library crate 😄 I personally think Chrono should aim to be comprehensive, and make things like this available, to avoid people having to spend time working it out and implementing it themselves.

@jtmoon79: ...but personally I also like... [start_of_, end_of_]

These are very much convenience functions, of course, but I would advocate adding something like this just because they are commonplace operations and it makes Chrono much easier to use. It also helps achieve more robst application code, with a more obvious purpose.

@pitdicker: NaiveDate::end_of_month ... I found a neat trick that seemed to fit in the spirit of chrono's design.

That sounds cool, and I will happily adjust my functions to work along those lines! 🙂

Constants

@jtmoon79: First, regarding new constants, bringing in constants related to newly introduced API functions is low-effort, low-risk, low maintenance, and just darn useful for users.

Agreed - although I don't necessarily think all of the constants I have would be needed, or in quite the same way. Many of my constants I added because there is no way to trivially get the information in constant fashion from the library. I think that first of all, the current Chrono constants should be public. I don't see the point of them being private - if Chrono uses them internally, the people using Chrono will often need to use the same building blocks, and that means either using those constants or recreating them themselves (which is what I have done), and then there is a danger of losing sync (which happened to me, and was how I discovered the limit changes).

Beyond that, I would propose that the range of constants be reviewed and, if necessary, renamed in places to present a complete range of useful foundational constants that Chrono relies on, and others can rely on. The *_FULL_* constants are the main area requiring scrutiny here.

So I think the constants I have in my crate are not exactly what I would propose being in Chrono, but some combination of the two 🙂

@pitdicker: I am not so sure about the Duration constants. The range of the type is almost 300 million years with nanosecond accuracy. Do we expect users to bump into its limitations so often it needs we need to add 20 constants describing its range?

I would say yes, although the cause is perhaps not readily apparent. The main place people bump into things like this is in tests. Any good programmer is going to want to create tests that ensure limits are handled correctly - yes, application Foo is likely never going to have a value that approaches some of the limits, but it might also not ever have an integer approaching the value of i64::MAX. Yet, i64::MAX is available, as it's good practice, and therefore can be trivially relied upon in tests. And, having such tests also implies that the application code will be made aware of the limits, so that it can check and handle them. As a use case, I gave an example further above about metrics which is relevant, but honestly I think writing reliable code and tests for that code are the number-one reason most people will have the need.

Another way of putting it is, if you don't intend people to use the full range, then reduce the range - but if you make the range available, then provide the interface necessary to use that range correctly.

@pitdicker: I guess you had to add these constants because before chrono 0.4.32 the initializers would panic on out of bound values. Do you still see a need for them now that we have alternatives that return an option?

I do, personally, albeit somewhat reduced. After all, we can now write code that checks for a None and therefore does not strictly have to check the constant values. However, that is often not desirable: it can be good to know why, so for instance report back a meaningful message such as, "your number 129 overflowed the limit of 128". My view is, give people more ways to interact, and let them choose - that may be different to yours; it's all subjective!

@pitdicker: Note that we already have Duration::max_value, and #1337 to make the methods on Duration const. So in the case you still need these constants you could write Duration::max_value().num_seconds() instead of MAX_SECONDS.

This is true, but: a) not all of those functions are const, and b) it is not as elegant or idiomatic. I personally do not like the use of MIN and MAX instances of Duration - I also don't like how they are implemented. It would make more sense to me, for example, if Duration::MAX could be used instead of Duration::max_value() - I find the functions are not obvious or idiomatic, and more verbose. General expectation is that there are simple constants for such things, and people know a constant is a constant, and that that won't change 🙂 Just my two cents! Ultimately I think this comes down to the inconsistencies you mentioned, and we should take steps to improve that.

Months

I don't have any comments here - I know @djc is not a fan of the type, and hence in my original post I suggested my functions here could be ignored.

General

@pitdicker: The hope is to reduce the API surface, make methods less panic-prone, use Result types and some nicer method names. But the chrono 0.5 branch is making little progress.

All of that sounds laudable - and I would love to help where possible 🙂 I think the API surface needs to be streamlined rather than simply reduced - some additions would be beneficial - and I definitely agree with removing panics!

@pitdicker: Chrono could use some more help, and you seem to be willing to contribute. Do you want to dive into some of the open PRs or issues?

Yes please 👍 I cannot commit much time right now - my recent Chrono-related work is only that which is demanded by another project that needs it - but I think I will be able to dedicate a few hours a week. This may well increase in future.

Interestingly, a couple of years or so ago I had idly thought about creating my own date and time library, as Chrono was stagnant at that point, but it's a lot to do and the need was not compelling. I am super-pleased that it has been revived, and the effort you guys have put in is great 🙂 It's much better to help make Chrono better now that it's active again!

@pitdicker: O, and have you found our discord?

I have indeed - I am a member there, although not very active. I felt this RFC was better-suited to a GitHub issue, but there are many day-to-day things that would be more suitable to discuss on Discord, for sure 👍

@jtmoon79: I really like this crate @danwilliams !

Thank you! 😄

Next steps

What I propose is this: if I start with the areas of most agreement, I can rustle up some PRs where we can discuss in detail the specifics of implementation of the thing in question. I'm worried that because this current issue covers a lot of ground, it has the potential to become unwieldy. Therefore, if I pick some areas, make some PRs, and link them back here, we can go over the details of how to get those things into Chrono in the best way possible as PR discussions? Does that sound like a good plan? 😁

[djc]

Duration

So in previous discussions I think my preferred direction at least was to avoid adding more stuff to Duration. In my mind Duration should morph into something maybe called TimeDelta, which is only used in chrono as an output value. For input values, IMO std::time::Duration should be used instead. So in general that means I'm not fond of adding more API to Duration that support the use as an input type.

@djc: NaiveDate::today() seems like a footgun to me because it has an implicit reliance on the local timezone. I think Local::now().date_naive() seems better.

I think this has been misunderstood - to me, it does not make sense to construct a NaiveDate using a local timezone: Utc would be the sensible default here. Therefore, that is why my code does Utc::now().date_naive(), i.e. Utc rather than Local. There are definitely things to be aware of, but nothing that can't be safeguarded with appropriate documentation (which prompts me that my own documentation for the function does not mention the timezone... ooops! 😊).

Well, that is exactly the point. Local::now().date_naive() and Utc::now().date_naive() are explicit about the perspective from which they decide what "today" means. A constructor like NaiveDate::today() would not, so it would prone to misuse (especially since in some timezones, there's a decent amount of overlap so the difference isn't always obvious) -- and no, I don't think documentation is a good way to improve on that since it's not usually as obvious while reading the code.

NaiveDate

@pitdicker: is_leap_year is supported since 0.4.30:

Interesting - I did not know that. I think my function predates it, and I'd not noticed leap_year() being added to perform the same role. Is there any reason behind the naming not following the more-common is_ prefix? Just curious.

@pitdicker: However once you start adding convenience methods like this, where do you draw the line? We currently have the line at little to none, and that is easy to defend.

Personally I like as many convenience functions to be available as will be useful to people interacting, whilst avoiding clutter. The easier people can achieve the task, they more they will like the library. This is a general statement, and not specific to Chrono.

You make this sound straightforward, but I disagree that it is. Even in your statement you mention "avoiding clutter", which I would definitely classify your range of _full() methods as. And then you're only taking into account the perspective of the user, whereas I think the maintainer perspective is also very important, and limiting in the context of a project that is dependent on volunteer maintenance.

@pitdicker: I wonder if it would be good to have an extension trait for everything-and-the-kitchen-sink convenience methods

Your idea of an extension trait is interesting - I would be happy to add some things in that way, if desirable, BUT I wonder is that convolution for the sake of a faulty ideal? Does it make it harder for people to find and use what they need? After all, the Rust compiler only builds in what's used, so presenting additional options to people is a question of documentation and discoverability, rather than performance.

Well, doesn't Rubedo already provide this? The helpful part of extension traits is that they don't have to live in the source crate.

Constants

@jtmoon79: First, regarding new constants, bringing in constants related to newly introduced API functions is low-effort, low-risk, low maintenance, and just darn useful for users.

Agreed - although I don't necessarily think all of the constants I have would be needed, or in quite the same way. Many of my constants I added because there is no way to trivially get the information in constant fashion from the library. I think that first of all, the current Chrono constants should be public. I don't see the point of them being private - if Chrono uses them internally, the people using Chrono will often need to use the same building blocks, and that means either using those constants or recreating them themselves (which is what I have done), and then there is a danger of losing sync (which happened to me, and was how I discovered the limit changes).

Beyond that, I would propose that the range of constants be reviewed and, if necessary, renamed in places to present a complete range of useful foundational constants that Chrono relies on, and others can rely on. The *_FULL_* constants are the main area requiring scrutiny here.

So I think the constants I have in my crate are not exactly what I would propose being in Chrono, but some combination of the two 🙂

I'm open to exposing more of the constants that are already being used, especially unopinionated ones.

@pitdicker: I am not so sure about the Duration constants. The range of the type is almost 300 million years with nanosecond accuracy. Do we expect users to bump into its limitations so often it needs we need to add 20 constants describing its range?

I would say yes, although the cause is perhaps not readily apparent. The main place people bump into things like this is in tests. Any good programmer is going to want to create tests that ensure limits are handled correctly - yes, application Foo is likely never going to have a value that approaches some of the limits, but it might also not ever have an integer approaching the value of i64::MAX. Yet, i64::MAX is available, as it's good practice, and therefore can be trivially relied upon in tests. And, having such tests also implies that the application code will be made aware of the limits, so that it can check and handle them. As a use case, I gave an example further above about metrics which is relevant, but honestly I think writing reliable code and tests for that code are the number-one reason most people will have the need.

Another way of putting it is, if you don't intend people to use the full range, then reduce the range - but if you make the range available, then provide the interface necessary to use that range correctly.

Pff, we just had a bunch of issues reported by users who were depending on the exact value of some of the limit constants, which I think goes to show that these APIs do get misused. I'm not saying we shouldn't have them, but I think you're optimistic on how they get used and how much exposing these contrains future API evolution.

General

@pitdicker: The hope is to reduce the API surface, make methods less panic-prone, use Result types and some nicer method names. But the chrono 0.5 branch is making little progress.

All of that sounds laudable - and I would love to help where possible 🙂 I think the API surface needs to be streamlined rather than simply reduced - some additions would be beneficial - and I definitely agree with removing panics!

Yes, the goal is to streamline the API, but that includes shrinking the surface for parts of the API that add little value.

Next steps

What I propose is this: if I start with the areas of most agreement, I can rustle up some PRs where we can discuss in detail the specifics of implementation of the thing in question. I'm worried that because this current issue covers a lot of ground, it has the potential to become unwieldy. Therefore, if I pick some areas, make some PRs, and link them back here, we can go over the details of how to get those things into Chrono in the best way possible as PR discussions? Does that sound like a good plan? 😁

Yes. 👍