Deprecate TimeZone::datetime_from_str

[pitdicker]

To quote #93 (comment):

From the documentation:
"If the to-be-parsed string includes an offset, it must match the offset of the TimeZone, otherwise an error will be returned."

So with TimeZone::datetime_from_str before parsing a string that includes an offset, you are expected to know the time zone. I.e. you have to know the offset before parsing a string as FixedOffset.

Also in the case of time zones like provided by chrono_tz parsing may fail even when you know the time zone, because the offset may have changed with a new revision of the time zone database.

I don't really see any use case where you would want to use TimeZone::datetime_from_str over DateTime::parse_from_str.

There is one case where it is currently used in tests and documentation that makes some sense: Utc.datetime_from_str. This allows parsing a string without offset values directly into a DateTime<Utc>.

Users would have now have to write NaiveDateTime::parse_from_str(/**/).and_utc(). I don't think this is much worse, it may even show more clearly what is happening.

[codecov]

Codecov Report

Merging #1251 (bbd7dd3) into 0.4.x (5a6b2b4) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            0.4.x    #1251      +/-   ##
==========================================
- Coverage   91.41%   91.37%   -0.04%     
==========================================
  Files          37       37              
  Lines       16453    16448       -5     
==========================================
- Hits        15041    15030      -11     
- Misses       1412     1418       +6     

Files Changed	Coverage Δ
src/offset/mod.rs	50.66% <ø> (-2.23%)	⬇️
src/datetime/tests.rs	96.41% <100.00%> (-0.10%)	⬇️
src/format/mod.rs	85.04% <100.00%> (ø)
src/format/parse.rs	97.24% <100.00%> (+<0.01%)	⬆️
src/lib.rs	95.58% <100.00%> (-0.07%)	⬇️
src/naive/datetime/tests.rs	98.65% <100.00%> (+0.10%)	⬆️

... and 1 file with indirect coverage changes

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

[djc]

Please split this into two commits. As a reminder, moving code and changing it in the same code makes it harder to review the changes.

[pitdicker]

Yes, of course.

[kryptan]

Top-level documentation still recommends datetime_from_str.

Also, deprecation warning points to DateTime::parse_from_str while NaiveDateTime::parse_from_str(...).and_utc() should actually be used as the OP suggests (parsing UTC time is probably the most common use case of parsing with known offset).

[tmccombs]

I have another use case:

I want to parse a string in the local time zone.

So I currently use:

Local.datetime_from_str(s, "%F %T")

But moving away from the deprecated function that becomes:

NaiveDateTime::parse_from_str(s, "%F %T").ok().and_then(|d| d.and_local_timezone(Local).single())

or possibly

NaiveDateTime::parse_from_str(s, "%F %T").ok()?.and_local_timezone(Local).single()

I have a couple complaints about this:

1. the deprecation note doesn't really help point to the solution. Using DateTime::parse_from_str is not a suitable replacement. And even if I were I writing new code, the documentation doesn't point to an obvious answer to "how do I parse a string in the current timezone?".
2. The above incantation involves three different types for fallible results. parse_from_str returns a Result, and_local_timezone returns a LocalResult, and single returns an Option.

[djc]

@tmccombs sorry you had a bad experience here.

1. Would you be able to send a PR to improve the deprecation note?
2. I agree this sucks, but I'm afraid for the 0.4 API there's not much we can do this due to semver compatibility constraints. For 0.5 improving this is definitely on the list, but that'll probably take some time to materialize.

[pitdicker]

I did not consider the use case of parsing a string without offset info to a known timezone, such as Local. Seems like an argument to un-deprecate and instead improve the documentation. @djc What do you think?

[tmccombs]

If doing that, then it may make sense to have it return an error if the format string includes an offset placeholder in the next major release.

[djc]

I did not consider the use case of parsing a string without offset info to a known timezone, such as Local. Seems like an argument to un-deprecate and instead improve the documentation. @djc What do you think?

Maybe a Local-specific API for this would be better than bringing back this semi-generalized API.

[seanlinsley]

Here's an example resolving the deprecation for iCal timestamp parsing, going from this to this:

fn from_timestamp(s: &str, tzid: Option<&str>) -> Result<DateTime<Utc>> {
  use chrono_tz::Tz;
  use std::str::FromStr;

  if let Ok(time) = NaiveDateTime::parse_from_str(s, "%Y%m%dT%H%M%S") {
    if let Some(tz) = tzid.map(|tz| Tz::from_str(tz).ok()).unwrap_or(Some(Tz::UTC)) {
      if let Some(time) = time.and_local_timezone(tz).single() {
        return Ok(time.with_timezone(&Utc));
      }
    }
  }

  bail!("failed to parse timestamp {}", s)
}

The documentation or deprecation warning could definitely be improved, at least.

[djc]

@seanlinsley feel free to submit a PR implementing any improvements you have in mind. Unfortunately I won't have time to work on that myself.