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.

Sign up for GitHub

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

Convert NaiveDate::from_weekday_of_month to return Result #1512

Merged
merged 1 commit into from
Mar 14, 2024
Merged

Convert NaiveDate::from_weekday_of_month to return Result #1512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 17 additions & 11 deletions src/naive/date/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,34 +334,40 @@ impl NaiveDate {
///
/// # Errors
///
/// Returns `None` if:
/// - The specified day does not exist in that month (for example the 5th Monday of Apr. 2023).
/// - The value for `month` or `n` is invalid.
/// - `year` is out of range for `NaiveDate`.
/// This methods returns:
/// - [`Error::DoesNotExist`] if the specified day does not exist in that month
/// (for example the 5th Monday of Apr. 2023).
/// - [`Error::InvalidArgument`] if the value for `month` or `n` is invalid.
/// - [`Error::OutOfRange`] if `year` is out of range for `NaiveDate`.
Comment on lines +338 to +341
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I guess these are ordered alphabetically, and I haven't really reviewed for this before, but IMO it would be nice to order these more semantically, roughly in the order they get checked? Like, it feels like InvalidArgument should go first here.

Copy link
Collaborator Author

@pitdicker pitdicker Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not order them alphabetically, but by how often I expect users to encounter them.
DoesNotExist should be quite normal, as it totally depends on the calendar. InvalidArgument shows the input is not checked for correctness before, which I think is less common. OutOfRange is for dates so many thousands of years into the past or future that I think them to be are real edge cases.

Does that sound like a reasonable order?

///
/// # Example
///
/// ```
/// use chrono::{NaiveDate, Weekday};
///
/// assert_eq!(
/// NaiveDate::from_weekday_of_month(2017, 3, Weekday::Fri, 2),
/// NaiveDate::from_ymd(2017, 3, 10).ok()
/// NaiveDate::from_ymd(2017, 3, 10)
/// )
/// ```
#[must_use]
pub const fn from_weekday_of_month(
year: i32,
month: u32,
weekday: Weekday,
n: u8,
) -> Option<NaiveDate> {
if n == 0 {
return None;
) -> Result<NaiveDate, Error> {
if n == 0 || n > 5 {
return Err(Error::InvalidArgument);
}
let first = try_opt!(ok!(NaiveDate::from_ymd(year, month, 1))).weekday();
let first = try_err!(NaiveDate::from_ymd(year, month, 1)).weekday();
let first_to_dow = (7 + weekday.number_from_monday() - first.number_from_monday()) % 7;
let day = (n - 1) as u32 * 7 + first_to_dow + 1;
ok!(NaiveDate::from_ymd(year, month, day))
// `day` may be larger than 31. That makes it an `InvalidArgument` for `from_ymd`, while in
// this context it just means that day in week 5 does not exist.
match NaiveDate::from_ymd(year, month, day) {
Ok(d) => Ok(d),
Err(_) => Err(Error::DoesNotExist),
}
}

/// Parses a string with the specified format string and returns a new `NaiveDate`.
Expand Down
28 changes: 15 additions & 13 deletions src/naive/date/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,19 +302,21 @@ fn test_date_from_num_days_from_ce() {
#[test]
fn test_date_from_weekday_of_month() {
let ymwd = NaiveDate::from_weekday_of_month;
assert_eq!(ymwd(2018, 8, Weekday::Tue, 0), None);
assert_eq!(ymwd(2018, 8, Weekday::Wed, 1), Some(NaiveDate::from_ymd(2018, 8, 1).unwrap()));
assert_eq!(ymwd(2018, 8, Weekday::Thu, 1), Some(NaiveDate::from_ymd(2018, 8, 2).unwrap()));
assert_eq!(ymwd(2018, 8, Weekday::Sun, 1), Some(NaiveDate::from_ymd(2018, 8, 5).unwrap()));
assert_eq!(ymwd(2018, 8, Weekday::Mon, 1), Some(NaiveDate::from_ymd(2018, 8, 6).unwrap()));
assert_eq!(ymwd(2018, 8, Weekday::Tue, 1), Some(NaiveDate::from_ymd(2018, 8, 7).unwrap()));
assert_eq!(ymwd(2018, 8, Weekday::Wed, 2), Some(NaiveDate::from_ymd(2018, 8, 8).unwrap()));
assert_eq!(ymwd(2018, 8, Weekday::Sun, 2), Some(NaiveDate::from_ymd(2018, 8, 12).unwrap()));
assert_eq!(ymwd(2018, 8, Weekday::Thu, 3), Some(NaiveDate::from_ymd(2018, 8, 16).unwrap()));
assert_eq!(ymwd(2018, 8, Weekday::Thu, 4), Some(NaiveDate::from_ymd(2018, 8, 23).unwrap()));
assert_eq!(ymwd(2018, 8, Weekday::Thu, 5), Some(NaiveDate::from_ymd(2018, 8, 30).unwrap()));
assert_eq!(ymwd(2018, 8, Weekday::Fri, 5), Some(NaiveDate::from_ymd(2018, 8, 31).unwrap()));
assert_eq!(ymwd(2018, 8, Weekday::Sat, 5), None);
assert_eq!(ymwd(2018, 8, Weekday::Tue, 0), Err(Error::InvalidArgument));
assert_eq!(ymwd(2018, 8, Weekday::Wed, 1), Ok(NaiveDate::from_ymd(2018, 8, 1).unwrap()));
assert_eq!(ymwd(2018, 8, Weekday::Thu, 1), Ok(NaiveDate::from_ymd(2018, 8, 2).unwrap()));
assert_eq!(ymwd(2018, 8, Weekday::Sun, 1), Ok(NaiveDate::from_ymd(2018, 8, 5).unwrap()));
assert_eq!(ymwd(2018, 8, Weekday::Mon, 1), Ok(NaiveDate::from_ymd(2018, 8, 6).unwrap()));
assert_eq!(ymwd(2018, 8, Weekday::Tue, 1), Ok(NaiveDate::from_ymd(2018, 8, 7).unwrap()));
assert_eq!(ymwd(2018, 8, Weekday::Wed, 2), Ok(NaiveDate::from_ymd(2018, 8, 8).unwrap()));
assert_eq!(ymwd(2018, 8, Weekday::Sun, 2), Ok(NaiveDate::from_ymd(2018, 8, 12).unwrap()));
assert_eq!(ymwd(2018, 8, Weekday::Thu, 3), Ok(NaiveDate::from_ymd(2018, 8, 16).unwrap()));
assert_eq!(ymwd(2018, 8, Weekday::Thu, 4), Ok(NaiveDate::from_ymd(2018, 8, 23).unwrap()));
assert_eq!(ymwd(2018, 8, Weekday::Thu, 5), Ok(NaiveDate::from_ymd(2018, 8, 30).unwrap()));
assert_eq!(ymwd(2018, 8, Weekday::Fri, 5), Ok(NaiveDate::from_ymd(2018, 8, 31).unwrap()));
assert_eq!(ymwd(2018, 8, Weekday::Sat, 5), Err(Error::DoesNotExist));
assert_eq!(ymwd(2018, 8, Weekday::Sat, 6), Err(Error::InvalidArgument));
assert_eq!(ymwd(2018, 13, Weekday::Sat, 1), Err(Error::InvalidArgument));
}

#[test]
Expand Down
14 changes: 10 additions & 4 deletions src/offset/local/windows.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ use std::ptr;
use super::win_bindings::{GetTimeZoneInformationForYear, SYSTEMTIME, TIME_ZONE_INFORMATION};

use crate::offset::local::{lookup_with_dst_transitions, Transition};
use crate::{Datelike, FixedOffset, LocalResult, NaiveDate, NaiveDateTime, NaiveTime, Weekday};
use crate::{
Datelike, Error, FixedOffset, LocalResult, NaiveDate, NaiveDateTime, NaiveTime, Weekday,
};

// We don't use `SystemTimeToTzSpecificLocalTime` because it doesn't support the same range of dates
// as Chrono. Also it really isn't that difficult to work out the correct offset from the provided
Expand Down Expand Up @@ -204,9 +206,13 @@ fn naive_date_time_from_system_time(
1..=5 => st.wDay as u8,
_ => return Err(()),
};
let date = NaiveDate::from_weekday_of_month(year, st.wMonth as u32, weekday, nth_day)
.or_else(|| NaiveDate::from_weekday_of_month(year, st.wMonth as u32, weekday, 4))
.ok_or(())?; // `st.wMonth` must be invalid
let date = match NaiveDate::from_weekday_of_month(year, st.wMonth as u32, weekday, nth_day) {
Ok(date) => date,
Err(Error::DoesNotExist) => {
NaiveDate::from_weekday_of_month(year, st.wMonth as u32, weekday, 4).unwrap()
}
Err(_) => return Err(()), // `st.wMonth` must be invalid
};
Ok(Some(date.and_time(time)))
}

Expand Down
Loading