0.4.x: deprecate methods that have an _opt() alternative

[djc]

As a compatible step towards a future for 0.5 where we try to not panic internally, this deprecates all methods that have an _opt() alternative in order to require the caller to unwrap() or expect() instead of the library doing that.

This is a small step towards #815 and allows us to test with current users how this direction is perceived.

[esheppa]

This looks excellent and I think this is the right incremental approach to get this done. Hopefully we will get some good feedback once 0.4.23 is released which can be incorporated into extensions on this in 0.5.

[djc]

Good feedback, thanks for catching the messy example situation -- I stumbled upon it late and then figured I'd ask for feedback before doing another round of cleaning that up. I think I've removed all the examples from the non-opt methods, and made sure that all _opt() methods have at least one example.

[esheppa]

Looks good! I've left a few comments in NaiveDate days iterator, also had one general thought:

where there are blocks like:

    /// assert_eq!(NaiveDate::from_ymd_opt(2015, 6, 3).unwrap().succ_opt(),
    ///            Some(NaiveDate::from_ymd_opt(2015, 6, 4).unwrap()));

potentially this style could be used instead:

    /// assert_eq!(NaiveDate::from_ymd_opt(2015, 6, 3).unwrap().succ_opt(),
    ///            NaiveDate::from_ymd_opt(2015, 6, 4)));

however, I don't think its worth doing those changes here as anyway, it may change further if the functions are changed to return result instead in some cases

[esheppa]

could we use self.value = current.succ_opt()?; and then remove the whole if block?

[djc]

I played with something like that at the time, but I don't think so? In the case succ_opt() returns None but current is Some we still have to return that last value. Arguably the iterator contract should be changed but I didn't want to take that on as part of this PR.

[esheppa]

sounds good - I just remembered why this stood out to me, I've made some changes to it in c7e1b9b anyway, so lets merge this as-is

[hoodie]

I have to admit that this makes for some rather verbose code.

[djc]

I have to admit that this makes for some rather verbose code.

Can you give some examples?

[hoodie]

the examples actually already contain that

let dt: NaiveDateTime = NaiveDate::from_ymd_opt(2016, 7, 8).unwrap().and_hms_opt(9, 10, 11).unwrap();

which used to be

let dt: NaiveDateTime = NaiveDate::from_ymd(2016, 7, 8).and_hms(9, 10, 11);

I completely understand the motivation of having a fallible default that doesn't panic, but for some cases (in my case just example code) it would have been acceptable to have the panicking version as long as it is well documented that it panics.

BTW: my first through when I saw that some of these return LocalResult<T> was "maybe I can just continue because LocalResult has the same functionality with the .and_hms():D "
That would have been cool. At least for a builder pattern

[djc]

Maybe that could make sense? Would you be able to submit a PR for something like that?

[esheppa]

@hoodie - I'm keen to understand the use cases for constructing dates from literals vs parsing them. If possible could you give a bit more detail about how/why you are using functions like from_ymd_opt and how often that occurs in your code base?. In chrono we construct dates/times like this a lot, mainly in tests and examples, but in my regular code I find myself doing this quite rarely.

In terms of continuing, the Option and Result helper methods mean you can do something like:

let dt: NaiveDateTime = NaiveDate::from_ymd_opt(2016, 7, 8).and_then(|d| d.and_hms_opt(9, 10, 11)).unwrap();

[hoodie]

@esheppa you are actually right. I also only use this in examples, which is why I was a bit disappointed that my own example code started looking a bit cumbersome after fixing these deprecations ;)

I don't think this needs to be fixed actually, it just makes example code a bit less sexy now

[tafia]

Would it be possible to reactivate it under certain conditions?

// not sure it actually works
#[cfg_attr(not(all(test, feature = "opt_unwrap")), deprecate)]

The rationale is

• this crate is used a LOT and testing a particular date is extremely common. The deprecation add lot of noise either in the code or the tests outputs
• I believe dates created in tests are overwhelmingly using valid dates and thus the opt is most of the time useless in that scenario
  • we're expecting a valid date
  • if this is not a valid date, test will panic which is actually ok!

EDIT; An alternative would be to use some sentinel value for dates (a bit like float, some not-a-date date). Not sure if this is better it'd just help with most common scenario (also I suppose, like for float may lead to some compiler optimizations?)

[djc]

@tafia there is some discussion in #815 about adding macros that could statically verify date/time validity.

I don't think doing a Cargo feature would make sense for this.

[njaard]

I really don't like the deprecation warning. Now I have to go through all my code and add _opt to hundreds of locations, then when chrono 0.5 comes out, I'll have to do it again, removing the _opt and adding an error check.

Considering that the breaking change in 0.5 will be a compile-time error, could I suggest that the deprecation warning be removed, or made as a feature flag?

[djc]

We'll see what we land on for 0.5 -- that will still take some time. For the time being, I don't think we're going to (conditionally) drop the deprecation warning, you can always introduce a wrapper in your own code if you're worried about churn.