0.5: Converting the API to return Results (part 2): `Mdf` type #1444

pitdicker · 2024-02-14T17:45:26Z

🎉

This and #1436 are the last ingredients before we can convert a good portion of the naive module to return Results.

In the first commit I adjusted four methods on Mdf to return a Result.

To ensure we correctly return either InvalidParameter or DoesNotExist on error, the MDL_TO_OL lookup table can now encode a second error case. We had 128 unused negative values available to do so, and it doesn't cost us any performance (compared to always returning the same variant).

The second commit changes NaiveDate::from_ymd_opt to return a Result to have something to benchmark against.

Benchmark

bench_date_from_ymd     time:   [4.5764 ns 4.5945 ns 4.6141 ns]

Compared to returning an Option this is a slight regression of 8% with main, and compared to 0.4.24 a regression of ca. 5%. In my opinion it stays in the right ballpark, and seems to be unavoidable.

Alternative

As an alternative I tried what happens if Mdf returns an Option, and we map it to an error in the callers. That brings ca. 20% overhead compared to main, and is a lot less nice to work with. Better to return Results everywhere.

cc @Zomtir

pitdicker · 2024-02-14T17:48:42Z

src/format/parsed.rs

@@ -390,7 +390,7 @@ impl Parsed {
        let (verified, parsed_date) = match (given_year, given_isoyear, self) {
            (Some(year), _, &Parsed { month: Some(month), day: Some(day), .. }) => {
                // year, month, day
-                let date = NaiveDate::from_ymd_opt(year, month, day).ok_or(OUT_OF_RANGE)?;
+                let date = NaiveDate::from_ymd_opt(year, month, day).map_err(|_| OUT_OF_RANGE)?;


This could be a proper mapping between the variants of Error and ParsedResult.
Because this will be one of the first places to touch when converting the parsing code (where it will become just ?), and because that code is still getting the preparations before we can convert it to Results, I'd like to wait with it until that time.

codecov · 2024-02-14T18:08:19Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (69208e5) 94.15% compared to head (00a73b1) 94.15%.

Files	Patch %	Lines
src/naive/internals.rs	89.47%	4 Missing ⚠️
src/error.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            0.5.x    #1444      +/-   ##
==========================================
- Coverage   94.15%   94.15%   -0.01%     
==========================================
  Files          37       37              
  Lines       16696    16701       +5     
==========================================
+ Hits        15720    15724       +4     
- Misses        976      977       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pitdicker · 2024-02-14T18:12:32Z

I may have forgotten the doc tests (among things) in the enthusiasm 😊. Sorry CI.

Zomtir

Looks very fine from what I see. I suggested a minor change to the -1/0/XX mapping.

Zomtir · 2024-02-15T12:57:31Z

src/lib.rs

@@ -129,20 +129,20 @@
 //! # fn doctest() -> Option<()> {


Last time I immediately changed the return type to Result<(), chrono::Error> so that I could make use of the ? as soon as possible. The rat tail is bigger than unwrapping intermediate results in the doctests, so I guess this works better for now and we have to clean up the tests once all functions return a Result.

What do you mean with 'rat tail'?

I don't look how the doctests are starting to look, but trust we can clean them up nicely after converting some more parts of the API.

Returning a result in this doctest requires too many changes at this point in time because most *_opt() functions still return an Option. "Rat tail" is a local saying for having to deal with too many consequences for a certain action.

Zomtir · 2024-02-15T14:57:43Z

src/naive/internals.rs

@@ -134,46 +136,47 @@ const MAX_MDL: u32 = (12 << 6) | (31 << 1) | 1;

 // The next table are adjustment values to convert a date encoded as month-day-leapyear to
 // ordinal-leapyear. OL = MDL - adjustment.
-// Dates that do not exist are encoded as `XX`.
+// Error cases are encoded as `-1` and `XX` (`0`). For a quick check all positive values are
+// adjustments, `XX` indicates a date does not exist, and negative values are for invalid input.
 const XX: i8 = 0;


It would be more coherent to either create a placeholder for both or neither. E.g.

const XX: i8 = 0; const IV: i8 = -1;

Actually I did just that but switched to this. It felt a bit confusing, looking like Roman numerals. I don't mind how we encode it. Maybe @djc has a preference?

Zomtir · 2024-02-15T15:08:00Z

src/naive/internals.rs

-        match MDL_TO_OL[mdl as usize] {
-            XX => None,
-            v => Some((mdl - v as u8 as u32) >> 1),
+        let adjustment = MDL_TO_OL[mdl as usize];


Would it be possible to keep the matching? That way the adjustment variable has a tighter scope.

Secondly x < 0 is not always the same as x == -1. Currently there is no other value than -1, but it is slightly more future proof to immediately detect -2 as a different error. A more suitable error would probably be Error::Undefined, but it is out of range in this case.

match MDL_TO_OL[mdl as usize] { adjustment if adjustment > 0 => Ok((mdl - adjustment as u8 as u32) >> 1), adjustment if adjustment == 0 => Err(Error::DoesNotExist), adjustment if adjustment == -1 => Err(Error::InvalidArgument), _ => Err(Error::OutOfRange), }

My first attempt used cmp(&0), but that didn't work in const context. I expect making the match more precise instead of positive/0/negative would effect the performance a bit. (Can't run a reliable benchmark on the device I am working on right now.)

Can we write it like this?

match adjustment { 1.. => Ok((mdl - adjustment as u8 as u32) >> 1), 0 => Err(Error::DoesNotExist), _ => Err(Error::InvalidArgument), }

Seems nice.

djc · 2024-02-16T17:30:39Z

src/naive/internals.rs

-        match MDL_TO_OL[mdl as usize] {
-            XX => None,
-            v => Some((mdl - v as u8 as u32) >> 1),
+        let adjustment = MDL_TO_OL[mdl as usize];


Can we write it like this?

match adjustment { 1.. => Ok((mdl - adjustment as u8 as u32) >> 1), 0 => Err(Error::DoesNotExist), _ => Err(Error::InvalidArgument), }

Seems nice.

djc · 2024-02-16T17:32:41Z

src/naive/date/mod.rs

        if year < MIN_YEAR || year > MAX_YEAR {
-            return None; // Out-of-range
+            return Err(Error::OutOfRange);


So from the perspective of this function, why isn't this InvalidArgument?

It seems to me a day that is 32 or a month that is 13 is just plain invalid. It is not allowed by the calendar system.

There is nothing particularly invalid about a year that is very far into the future. It is a limitation that Chrono has where we are hitting the (very reasonable) range of our types.

pitdicker · 2024-02-16T19:07:43Z

🎉 Nice.

@Zomtir If you want to help out the time has come. Drop a note if you start something so I'll steer clear 😄.

It seems good to do the conversion with only one method or a very small number of methods per commit. And starting with methods in the naive module that don't depend on pieces that have not yet been converted.

Converting LocalResult, ParseError and TimeDelta require some preparations first that I'll look into. Let's ignore anything around those for now.

Zomtir · 2024-02-17T17:24:46Z

Drop a note if you start something so I'll steer clear

I started with NaiveDate and got it to compile: 5d476d3

Docs are not updated yet.

pitdicker · 2024-02-17T19:08:54Z

Nice! That requires surprising little changes. I hope you can split it up in smaller commits.
Take your time to make the docs, tests and doctests work and more specific, now that they can be.

Could you skip the DateTime type? The map_local type is going to pass on multiple error causes, such as OutOfRange, DoesNotExist, OS errors, and trouble with a TZ string or TZif file. I'd like to be a bit more careful with that type and leave that to its own PR.

We should also have a discussion around the Datelike and Timelike traits before converting them. For the naive types their signature is fine. For DateTime it would be better if they returned LocalResult to give users a way to deal with timezone issues (in my opinion). See #1050.

pitdicker commented Feb 14, 2024

View reviewed changes

pitdicker force-pushed the mdf_result branch from 9de6309 to 359c524 Compare February 14, 2024 18:04

pitdicker force-pushed the mdf_result branch from 359c524 to d0d929f Compare February 14, 2024 18:11

Zomtir reviewed Feb 15, 2024

View reviewed changes

djc reviewed Feb 16, 2024

View reviewed changes

pitdicker added 2 commits February 16, 2024 18:51

Convert Mdl to return Result

fc9315f

Convert NaiveDate::from_ymd to return Result

00a73b1

pitdicker force-pushed the mdf_result branch from d0d929f to 00a73b1 Compare February 16, 2024 18:29

djc approved these changes Feb 16, 2024

View reviewed changes

pitdicker merged commit 8c5425d into chronotope:0.5.x Feb 16, 2024
35 checks passed

pitdicker deleted the mdf_result branch February 16, 2024 18:59

Zomtir mentioned this pull request Feb 17, 2024

0.5: Convert API to return Result: naive module #1445

Closed

Zomtir mentioned this pull request Feb 28, 2024

0.5: Convert API to return Result: Datelike::with_year #1466

Merged

pitdicker mentioned this pull request Feb 28, 2024

Tracking issue: Converting the API to return Results #1469

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

0.5: Converting the API to return Results (part 2): `Mdf` type #1444

0.5: Converting the API to return Results (part 2): `Mdf` type #1444

pitdicker commented Feb 14, 2024 •

edited

Loading

pitdicker Feb 14, 2024

codecov bot commented Feb 14, 2024 •

edited

Loading

pitdicker commented Feb 14, 2024

Zomtir left a comment

Zomtir Feb 15, 2024

pitdicker Feb 15, 2024

Zomtir Feb 15, 2024

Zomtir Feb 15, 2024 •

edited

Loading

pitdicker Feb 15, 2024

Zomtir Feb 15, 2024 •

edited

Loading

pitdicker Feb 15, 2024

djc Feb 16, 2024

pitdicker Feb 16, 2024

djc Feb 16, 2024

djc Feb 16, 2024

pitdicker Feb 16, 2024

pitdicker commented Feb 16, 2024

Zomtir commented Feb 17, 2024

pitdicker commented Feb 17, 2024 •

edited

Loading

0.5: Converting the API to return Results (part 2): Mdf type #1444

0.5: Converting the API to return Results (part 2): Mdf type #1444

Conversation

pitdicker commented Feb 14, 2024 • edited Loading

🎉

Benchmark

Alternative

Choose a reason for hiding this comment

codecov bot commented Feb 14, 2024 • edited Loading

Codecov Report

pitdicker commented Feb 14, 2024

Zomtir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zomtir Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zomtir Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitdicker commented Feb 16, 2024

Zomtir commented Feb 17, 2024

pitdicker commented Feb 17, 2024 • edited Loading

0.5: Converting the API to return Results (part 2): `Mdf` type #1444

0.5: Converting the API to return Results (part 2): `Mdf` type #1444

pitdicker commented Feb 14, 2024 •

edited

Loading

codecov bot commented Feb 14, 2024 •

edited

Loading

Zomtir Feb 15, 2024 •

edited

Loading

Zomtir Feb 15, 2024 •

edited

Loading

pitdicker commented Feb 17, 2024 •

edited

Loading