0.5: Convert API to return Result: naive module

[Zomtir]

Follow up commit for #1444.

• Changes the NaiveDate, NaiveTime and NaiveDateTime to return Result
• Depends on the Datelike and Timelike traits to return Result
• Consequently DateTime<Tz> has to return Result as well

Doc and tests still WIP.

Feedback from @pitdicker

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.

Do smaller commits have to compile? Currently this is a minimum patch to be able to compile.

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.

DateTime implements the Datelike and Timelike traits. Doesn't this mean I have to change all classes that implement that trait? Please call me out on that if I am wrong.

    fn with_year(&self, year: i32) -> Result<DateTime<Tz>, Error> {
        map_local(self, |datetime| datetime.with_year(year))
    }

If I ignore map_local, the call stack would be Result<Datelike> -> Option<Datelike> -> Result<Datelike>. This will hide the error and not propagate the earlier one. The only solution I came up with was to adapt map_local to pass the error as Result.

The consequence was that all functions that make use of map_local would need to propagate a Result.

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.

This commit covers quite a lot of the changes mentioned in #1050. Wouldn't this patch be the perfect groundwork for #1050? The commits do not really conflict and this patch only retains current behaviour.

[pitdicker]

As I see it there are two ways to go about converting the API to return Results, and we don't seem to be on the same page yet.

Option 1: Convert many function signatures first, do any necessary refactorings, code changes, and documentation later.
Option 2: Do necessary refactorings, code changes, and documentation first, convert signatures a couple at a time.

I have a preference for option 2, for a couple of reasons.

• The 0.4.x and 0.5.x branches are probably going to exist for quite some time side by side. The longer and the more code we can keep shared between them, the better. And shipping part of the code before a big 0.5 release will help to catch issues. With option 2 it is easier to do part of the work on the main branch, and only do the last step on 0.5.x branch.

• By only converting methods once all the methods they call are converted, we can make sure and test they return the correct error. And the type system helps to make sure we don't miss any of their call sites but adjust them to do the correct thing to pass on their errors.

• Splitting the work in small steps makes reviewing the changes more manageable.

As you may have noticed I already started working in the pattern of option 2.
Preparations for converting NaiveDate landed on main in #1427, #1328, #1429, #1431, #1432, #1433, #1207 and #1438. #1435 and #1443 merged that with the 0.5.x branch, and #1436 was one preparation on the 0.5.x branch. Only then was the switch in #1444.
In the same vein I am working on the parsing module at the moment.

Do smaller commits have to compile? Currently this is a minimum patch to be able to compile.

They can if you use adapters to convert from Result to Option, such as Result::ok() or the ok! macro as in #1410 and #1444.

DateTime implements the Datelike and Timelike traits. Doesn't this mean I have to change all classes that implement that trait? Please call me out on that if I am wrong.

That is why I gave the suggention in #1444 (comment):

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.

The trouble is that I have a bit of a dependency tree in my mind to split up the work or to bring up tricky area's for discussion, but that is not all written down yet. I'll do some of that today.

I hope this doesn't discourage you. If it is any consolation: I already tried converting most methods in the formatting module twice, and am going to have to do it a third time.

[Zomtir]

As I see it there are two ways to go about converting the API to return Results, and we don't seem to be on the same page yet.

Option 1: Convert many function signatures first, do any necessary refactorings, code changes, and documentation later. > Option 2: Do necessary refactorings, code changes, and documentation first, convert signatures a couple at a time.

Especially for this commit the documentation changes are reliant on the signature change, so Option 2 seems not to be a viable option.

Do smaller commits have to compile? Currently this is a minimum patch to be able to compile.

They can if you use adapters to convert from Result to Option, such as Result::ok() or the ok! macro as in #1410 and #1444.

As mentioned, map_local will hide error types with that approach. I would have to create a Error::TempError when going back from Option to Result with ok_or(Err).

DateTime implements the Datelike and Timelike traits. Doesn't this mean I have to change all classes that implement that trait? Please call me out on that if I am wrong.

That is why I gave the suggention in #1444 (comment):

That is not addressing the problem. I cannot change the signature of an implementation without changing the signature of the trait. Whether or not DateTime should implement Datelike seems to be an ongoing point of discussion and shouldn't be used as a blocker for converting the signature.

The pure API changes are roughly +100/- 100. The documentation change per function is about +120/-200 per function. There are only 4 (7) functions affected by that commit.

• with_year
• with_month(0)
• with_day(0)
• with_ordinal(0)

I can help with converting the signature, adapting the tests and improving the documentation. But functional refactoring is beyond my current level of time commitment. Core contributors have a better global understanding of the targeted behaviour which I have not.

My personal stance is that the signature changes facilitate the refactoring due to more verbose error handling and better accessibility.

I will finish this commit with the doc changes for with_day and with_ordinal and would be glad if you could a quick look. All in all it is not drastic and hope it is considered being picked up.

[Zomtir]

I now see what you mean. It is blowing up at the moment because I also changed with_hour, with_minute etc...

Doesn't mean I have any idea how to get around map_local. I'll see what magic I come up with.

[pitdicker]

The issue is that I hoped to do this slow but right, instead of clearing the bar 'it compiles', and fix later.

Especially for this commit the documentation changes are reliant on the signature change, so Option 2 seems not to be a viable option.

It is viable 😄. For the methods in the naive module the documentation changes that are relevant for 0.4.x have already happened in #1120. With this PR it now only needs to be updated to match the new signatures.

That is not addressing the problem. I cannot change the signature of an implementation without changing the signature of the trait.

True. That is why I suggested not touching the trait in this PR yet, and wait a bit.

I can help with converting the signature, adapting the tests and improving the documentation. But functional refactoring is beyond my current level of time commitment.

I am glad with your help, and hope to do the preparations myself. Once a new area is ready to convert we can divide up the work depending on your time.

[Zomtir]

True. That is why I suggested not touching the trait in this PR yet, and wait a bit.

Rather than replacing map_local, I added a temporary variant map_local_result. This neatly works around the dependency network and allows one function signature change after another.

80f7278

It still touches the Datelike trait unfortunately. I can wait until this is addressed first, but this seems like a good compromise and supports the efforts targeted in #1050 (besides dropping the trait).

[pitdicker]

@Zomtir No need to hurry. But as I saw you closed this PR... How are you doing?

[Zomtir]

@pitdicker Slight progress visible here: https://github.com/Zomtir/chrono/tree/with_year

I'm trying to be exact with the return types in the documentation, but an exhaustive list of error will produce quite a lot of duplicated information. I'm trying to document the important nodes (and their errors) in the call chain and reference them from the callers instead.

The LocalResult is one of those intermediate nodes. It felt like I head to skip ahead of #1448 and introduce a temporary mapping. A central mapping looked more compatible to the LocalResult rework than to introduce a local mapping in DateTime. Ideally DateTime::map_local and its callers will not be affected by #1448.

All in all, not yet ready for review sadly.

[pitdicker]

May I suggest the following methods as a first step, hopefully spread over multiple PRs?

• NaiveDate::from_num_days_from_ce
• NaiveDate::from_weekday_of_month
• NaiveDate::checked_add_months
• NaiveDate::checked_sub_months
• NaiveDate::checked_add_days
• NaiveDate::checked_sub_days
• NaiveDate::succ
• NaiveDate::pred
• NaiveDate::checked_add_signed
• NaiveDate::checked_sub_signed
• NaiveDate::years_since

They should be easier to convert, and be a good basis for the rest of the work.

[pitdicker]

Especially from_num_days_from_ce is useful to convert the timestamp-related methods.

[Zomtir]

@pitdicker

I looked at a few of those:

Dependency of NaiveDate::from_num_days_from_ce

• NaiveDate::from_ordinal_and_flags

Dependency of NaiveDate::checked_add_days

• NaiveDate::add_days
• NaiveDate::from_ordinal_and_flags
• i32::checked_add

I guess the best would be to start with NaiveDate::from_ordinal_and_flags. If you have a preferred way to handle i32::checked_add, let me know.

[pitdicker]

Dependency of NaiveDate::from_num_days_from_ce

* `NaiveDate::from_ordinal_and_flags`

#1460 takes care of from_ordinal_and_flags. I'll prepare a merge of the main branch with 0.5.x, so we can finish that one.

Edit: Want to do NaiveDateTime::from_timestamp* and DateTime::from_timestamp* in the same PR?

[pitdicker]

NaiveDate::add_days seems natural to convert together with NaiveDate::checked_add_days and related methods.

[pitdicker]

If you have a preferred way to handle i32::checked_add, let me know.

Not yet 😄. ok_or is not yet available in const context? Maybe good to have have a function or macro with a similar name in lib.rs (with the other error macro's). I count 11 places where we could use such a macro eventually.

[Zomtir]

Not yet 😄. ok_or is not yet available in const context? Maybe good to have have a function or macro with a similar name in lib.rs (with the other error macro's). I count 11 places where we could use such a macro eventually.

Yes, ok_or is not available. But even if it was, a macro or helper function would be preferable over doing the same thing 11 times. In c7a5517 I used matching for NaiveDate::from_num_days_from_ce.

What is the procedure for commits that depend on pending merges? I based my branch on your d60fbaf, so those would show up in the pull request against 0.5.x.

[Zomtir]

Edit: Want to do NaiveDateTime::from_timestamp* and DateTime::from_timestamp* in the same PR?

Ctrl-F from_timestamp: 115 results... well let's hope this stays stays reviewable. I will start.

[pitdicker]

Just yesterday I did FixedOffset::{east, west}, with 119 results. Only a couple of lines needed adjusting. So maybe you are lucky.

[pitdicker]

What is the procedure for commits that depend on pending merges?

Usually I have a queue of branches ready to open as a PR once the work it builds on is merged 😆.
You can now rebase on 0.5.x.