Add DateTime::fix_offset

[pitdicker]

As an anecdote, I was stumped for days how to let a hobby project handle timestamps with timezones. Entries could come from various sources, with different associated timezones. So I had generic DateTime<Tz: Timezone> objects. How to collect those in a data structure?

Finally it 'clicked' that the full timezone didn't matter in my case, just the offset from UTC of an entry should be preserved.

Converting a generic DateTime to DateTime<FixedOffset> is trivial when you know the innards of chrono. But if there was a Datetime::fix_offset method it probably would have set me on the right track. (Then this took me quite a while because the fix() method is somewhat hidden in the Offset trait, and I saw a similar story in one of the issues.)

Note that a generic From implementation is not possible:

impl<Tz: TimeZone> From<DateTime<Tz>> for DateTime<FixedOffset> {
    fn from(src: DateTime<Tz>) -> Self {
        src.with_timezone(&src.offset().fix())
    }
}

That fails with conflicting implementation in crate 'core': impl<T> From<T> for T.

I also remember one of my naive tries was with_timezone(&FixedOffset), comparable to with_timezone(&Utc). But that doesn't just work, the FixedOffset must be derived from the DateTime first.

What a fix_offset method would also be an alternative to is the unfortunate implementation of
From<DateTime<Local>> for DateTime<FixedOffset>, that says: "Note that the converted value returned by this will be created with a fixed timezone offset of 0."

I am curious about your opinions. Can converting a generic DateTime to DateTime<FixedOffset> be considered a fundamental enough thing to do to add this method?

[pitdicker]

Reading through other issues/PRs, I see a method like this was suggested before by @esheppa in #829 (comment).

[pitdicker]

Also suggested by @epipheus in #624 (comment).

[pitdicker]

The closed PR #305 had a similar motivation.

[jtmoon79]

LGTM

[jtmoon79]

Reading through other issues/PRs, I see a method like this was suggested before by @esheppa in #829 (comment).

Also suggested by @epipheus in #624 (comment).

The closed PR #305 had a similar motivation.

This is getting at the biggest difficulty in this entire crate: converting between datetime types.
When 0.5 is released, I plan to put together a "chrono cheat sheet". A big XY table of conversions and their recipes.

[pitdicker]

When 0.5 is released, I plan to put together a "chrono cheat sheet". A big XY table of conversions and their recipes.

I may have made just that for the DateTime type: some documentation on how to create it, which actions are available, and how to convert it to other types. Will have a PR probably later today 😄. It is a first step to expand the documentation, which was my biggest trouble with chrono.

[jtmoon79]

When 0.5 is released, I plan to put together a "chrono cheat sheet". A big XY table of conversions and their recipes.

I may have made just that for the DateTime type: some documentation on how to create it, which actions are available, and how to convert it to other types. Will have a PR probably later today 😄. It is a first step to expand the documentation, which was my biggest trouble with chrono.

I was imagining it might be fun to use a markdown table where the XY meeting point has a link to the code recipe.

| ↓ From \ To →           | `DateTime<FixedOffset>`               | `Local`                            | `Utc`                            | `FixedOffset`                            | `Timezone`                            |
|-------------------------|---------------------------------------|------------------------------------|----------------------------------|------------------------------------------|---------------------------------------|
| `DateTime<FixedOffset>` |                                       | [#](#datetimefixedoffset_to_local) | [#](#utc_to_datetimefixedoffset) | [#](#fixedoffset_to_datetimefixedoffset) | [#](#timezone_to_datetimefixedoffset) |
| `Local`                 | [#](#local_to_datetimefixedoffset)    |                                    | [#](#utc_to_local)               | [#](#fixedoffset_to_local)               | [#](#timezone_to_local)               |
| `Utc`                   | [#](#utc_to_datetimefixedoffset)      | [#](#utc_to_local)                 |                                  | [#](#fixedoffset_to_utc)                 | [#](#timezone_to_utc)                 |
| `FixedOffset`           | [#](#fixed_to_datetimefixedoffset)    | [#](#fixedoffset_to_local)         | [#](#utc_to_fixedoffset)         |                                          | [#](#timezone_to_fixedoffset)         |
| `Timezone`              | [#](#timezone_to-datetimefixedoffset) | [#](#timezone_to_local)            | [#](#utc_to_timezone)            | [#](#fixedoffset_to_timezone)            |                                       |

Would look something like this (incomplete) example

---

↓ From \ To →	DateTime<FixedOffset>	Local	Utc	FixedOffset	Timezone
DateTime<FixedOffset>		#	#	#	#
Local	#		#	#	#
Utc	#	#		#	#
FixedOffset	#	#	#		#
Timezone	#	#	#	#

Local to DateTime<FixedOffset>

use chrono::{DateTime, Local, FixedOffset};

let local = Local::now();
let dt: DateTime<FixedOffset> = local.into();

Utc to DateTime<FixedOffset>

// code recipe

FixedOffset to DateTime<FixedOffset>

// code recipe

... (more code recipes linked in the table above) ...

[djc]

Can you add some basic tests for this?

[pitdicker]

Can you add some basic tests for this?

Intended to do so this morning, I have mostly learned the lesson by now 😆.

I added a line to the test test_datetime_local_from_preserves_offset to test from Local, and added a separate test for the conversion from Utc and FixedOffset (which does nothing).

[esheppa]

Sorry to nitpick, but should this be with_fixed_offset to match the naming conventions?

[pitdicker]

I don't mind. I picked fix_offset because it is similar to the current Offset::fix. It 'fixes' the offset to its current value.

I would expect that with a method with the name with_fixed_offset you have to supply an argument, like you have to with with_hour, with_year, etc.

[djc]

I agree with @pitdicker that the with_ convention doesn't fit here, as that is usually used to augment the existing value with new data which is not the case here.

On the other hand, after this comment I did wonder if fixed_offset() would be a better name, on the notion that this doesn't mutate (which fix_ suggests to me) but instead returns the new value.

[pitdicker]

I found myself typing it that way (fixed_offset()) by mistake a few times.
But the method name should have a verb in it.

To continue bikeshedding: what about to_fixed_offset? It doesn't return a FixedOffset, but returning a DateTime<FixedOffset> does not seem too surprising to me.

[djc]

fixed_offset() still does have a verb in it, just in past tense (I'm not convinced by this argument).

The fact that you mistyped it a few times seems more evidence that fixed_offset() is the correct spelling.

I think the ecosystem convention is that a to_fixed_offset() function should yield a FixedOffset, which doesn't fit here.

[pitdicker]

Nobody can say we didn't think about naming 😆. Changed it to fixed_offset.

[esheppa]

Looks great!