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

Add NaiveDate::diff_months_days #1247

Closed
wants to merge 1 commit into from

Conversation

pitdicker
Copy link
Collaborator

This is my oldest branch of chrono, that never got turned into a PR. From ca. 1½ year ago.
I kept putting it off, because arguing for the utility of something is not my hobby 😄.

NaiveDate::leap_year()

Sometimes you just need to know whether the current date falls in a leap year.

A naive way to determine this is to do it manually:

let year = date.year();
let is_leap = year % 4 == 0 && (year % 100 != 0 || year % 400 == 0);

But that kind of defeats the purpose of using chrono, and it involves 3 divisions.

Better to do:

let is_leap = date.with_ordinal(366).is_some();

But I would say that is pretty creative API use, that few will come up with.

It also involves some work, while the information is right there as a flag in NaiveDate. I propose to expose it with NaiveDate::leap_year().

See #29 and #69 for related issues.

NaiveDate::diff_months_days()

For a project I needed to know the number of whole months and remaining days between two dates.
This turns out to be a bit difficult, until you know how to look at the problem.

The correct answer is different if you are counting down towards a date, compared to counting the number of months and days since an event.
In both cases there is a reference date, and when using this method as reference.diff_months_days(other) you get the right answer without having to think about it much.

NaiveDate::diff_months_days provides a human way to talk about larger durations: "there are 4 months and 13 days between those dates". The days value can then of course be rounded to something like weeks without much effort: "there are 4 months and 2 weeks between those dates". Or "4½ months".
Also the value of months can be turned into years and months without much effort.

But the difficult part, getting months and days, is what NaiveDate::diff_months_days is for.

@codecov
Copy link

codecov bot commented Sep 2, 2023

Codecov Report

Merging #1247 (1f6287a) into 0.4.x (f4aefc7) will increase coverage by 0.00%.
The diff coverage is 94.87%.

@@           Coverage Diff           @@
##            0.4.x    #1247   +/-   ##
=======================================
  Coverage   91.38%   91.39%           
=======================================
  Files          37       37           
  Lines       16467    16506   +39     
=======================================
+ Hits        15049    15086   +37     
- Misses       1418     1420    +2     
Files Changed Coverage Δ
src/naive/date.rs 96.32% <94.87%> (-0.03%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pitdicker
Copy link
Collaborator Author

Code coverage is not happy, #1248 should help.

@pitdicker
Copy link
Collaborator Author

#416 seems to be a related request and can be solved with diff_months_days as a building block.

@djc
Copy link
Member

djc commented Sep 6, 2023

I think I'm open to something like leap_year(). My question is whether leap_year() is the most ergonomic formulation or if we should have year_days() -> u16 (name TBD) instead. I'm much less convinced of the benefit of diff_month_days().

@pitdicker
Copy link
Collaborator Author

My question is whether leap_year() is the most ergonomic formulation or if we should have year_days() -> u16 (name TBD) instead.

I prefer just leap_year(). I have a couple of cases were I use something like this were I do not need to know the number of days in the year, but to map an ordinal or to know the number of days in februari.

I'm much less convinced of the benefit of diff_month_days().

I'll split it out leap_year() into another PR, and we can discuss the other a bit more here (I hope).

@pitdicker pitdicker changed the title Add NaiveDate::leap_year and NaiveDate::diff_months_days Add NaiveDate::diff_months_days Sep 6, 2023
@pitdicker
Copy link
Collaborator Author

pitdicker commented Sep 7, 2023

@djc What would you say if instead of this PR I work on adding

struct CalendarPeriod {
    months: u32,
    days: u32,
    time: core::time::Duration,
}

from your comment in #579 (comment)?

In other words create a type with full ISO 8601 duration support (I think I can do something reasonable with fractions also).
We can implement most of the methods of https://crates.io/crates/date_component on it.

I would like to start with adding the type and basic operations first, and add parsing and formatting later.

@djc
Copy link
Member

djc commented Sep 8, 2023

That sounds much more interesting to me, particularly because I would like to excise https://github.com/PoiScript/iso8601-duration from my dependency trees. (At least that was used by async-graphql before -- apparently it is now using https://docs.rs/iso8601/latest/iso8601/.)

@pitdicker
Copy link
Collaborator Author

Closing. I'll be back 😄

@pitdicker pitdicker closed this Sep 14, 2023
@pitdicker pitdicker mentioned this pull request Sep 14, 2023
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants