Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

resolve a number of outstanding breaking changes (the path to jiff 0.2) #217

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

BurntSushi
Copy link
Owner

@BurntSushi BurntSushi commented Jan 25, 2025

This is a WIP PR meant to resolve all of the outstanding issues labeled
breaking change. The idea is to bring us very close to releasing
jiff 0.2.

If all goes well with jiff 0.2, then my plan will be to release jiff 1.0
in Summer 2025. Unless a major flaw is found, I plan to commit to
jiff 1.x.y indefinitely.

@BurntSushi BurntSushi force-pushed the ag/0.2 branch 9 times, most recently from 9b81ac6 to 90dbcbc Compare January 30, 2025 02:38
Previously, it was an API bug for this routine to be infallible since we
specifically disallow adding calendar spans on `Timestamp`. To paper
over this, we made the API panic in such cases. But now we properly
return an error.

It is lamentable that an API like this is fallible. And the generics
means that even adding a `SignedDuration`, which ought to be infallible,
callers still need to deal with the possible error path.

Fixes #36
This officially drops the deprecated behavior of `%V`, which
corresponded to an IANA time zone identifier. This was done to improve
compatibility with other `strftime` and `strptime` implementations, such
as the one found in GNU libc.

Fixes #147
These have been replaced by `in_tz` methods.

Closes #28
These were deprecated. Users should use `Date::iso_week_date` and
`ISOWeekDate::date` instead.

Fixes #218
This will cause Jiff to reject some datetime strings that it
previously accepted. This also brings Jiff into line with how
Temporal behaves in these case. It is overall safe because
it avoids misinterpreting datetimes in the future that are
serialized before a change to DST. For example, imagine serializing
`2006-04-02T02:30-05[America/Indiana/Vevay]` before you knew that
Indiana/Vevay was going to switch to DST (they hadn't previously used
DST since 1972). Before that switch, it was perfectly reasonable to
think that the datetime was valid. But after the switch, it corresponded
to a gap. Jiff will now reject this.

Users can still of course change the offset conflict resolution strategy
depending on what they want to do (reject, keep exact time or keep civil
time).

Fixes #212
This makes the naming with APIs like `Zoned::duration_since` a bit more
consistent. And solidifies `SignedDuration` as Jiff's primary
non-calendar duration type.

I'm open to adding `Span::to_unsigned_duration` in the future, but let's
see how far we can get with what we have. Note that
`TryFrom<Span> for std::time::Duration` still exists, although that of
course only works for non-calendar spans since there is no way to
provide a relative reference time.

Fixes #220
Jiff has three different kinds of databases that it can draw time zone
transitions from: the standard Unix zoneinfo database, a bundled or
embedded zoneinfo database (compiled into the binary), or the special
Android concatenated zoneinfo database. Previously, we would try to
create as many of these databases as possible, and then look all time
zones up in each.

I think that this type of semantic is very messy, because you can wind
up drawing from one db for one time zone and another db for another
(although in theory this shouldn't happen if they're all in sync). It
also requires that you always look for all three, which feels wrong.

Instead, we now just look for one and stop when we find it. Effectively,
we changed the internals from a product to a sum.

On Android, we check for a concatenated tzdb first, since that's likely
what we'll find.

Fixes #213
This should be more friendly to future API evolution here. I previously
did the simplest thing possible and wasn't really thinking about
core-only environments.

We still don't support any tzdb in core only environments, but we
theoretically could in the future. (Although there are a number of
issues to figure out. This merely fixes one of them.)

Fixes #221
This started with wanting to get rid of the `(offset, dst, abbrev)`
triple in the `to_offset` API. Specifically, I thought that returning
a `&str` tied to the time zone for the abbreviation was somewhat
limiting from an API evolution perspective, particularly in core-only
environments. Specifically, in core-only environments, we only support
fixed offset time zones, and in order to provide the `&str` API, we had
to pre-compute the string representation of that offset and store it on
the `TimeZone`. Since there's no indirection in `TimeZone` in core-only
environments, this bloated the size of it, and consequently `Zoned` as
well.

By changing this to an opaque type with a more restrictive lifetime, we
can compute the string "later."

But... we really don't want to be doing this every single time we want
the offset for an instant. And moreover, even in non-core environments,
this code path was doing more work than is necessary (albeit not much)
to return the DST status and time zone abbreviation strings. It's also
just more data to shuffle around.

So I split `to_offset` into two APIs: one for common use cases and one
that returns the data that `to_offset` returned previously. This
required a fair bit of rejiggering, but nothing substantial changed.

Also, I changed the lifetime of the abbreviation returned by
`TimeZoneTransition::abbreviation` to be tied to the transition. This
also affords us some API flexibility in the future for similar reasons
as above.

Fixes #222
This makes the `Span` APIs more consistent with the rest of Jiff in that
days are never silently assumed to be 24 hours. Instead, callers are
forced to either provide a relative date or a special
`SpanRelativeTo::days_are_24_hours()` marker to opt into invariant
24-hour days.

This is meant to make the API more uniform and reduce the possibility of
bugs. In particular, in #48, it was brought up that it is very easy to
assume that Jiff APIs will never let you silently assume that days are
always 24 hours. This is actually quite easy to do if you're just
dealing with `Span` APIs.

Fixes #48
Basically, if callers opt into days being invariant---and thus no
relative date is required to resolve its length---then weeks are also
treated as invariant.

Temporal doesn't do this. As I understand it, I think the reasoning is
that they might some day support calendars that don't use 7 day weeks.
However, I think it's still pretty unlikely for Jiff to support
non-Gregorian calendars (other than things like the ISO 8601 week date
calendar). And moreover, I believe the only calendars to use weeks that
aren't 7 days are ancient calendars. I believe all actively used
calendars use 7 day weeks. So for this assumption to be wrong, Jiff
would not only need to support non-Gregorian calendars but _ancient_
non-Gregorian calendars. I think that's probably never going to happen
and is best left to specialty crates.

Because of that, I think it's say to support invariant weeks *when* the
caller opts into invariant days.

Closes #136
Basically, instead of just logging a warning and falling back to `UTC`,
we now still log a warning and fall back to a time zone that _behaves_
as `UTC`, but is identified as `Etc/Unknown`. This avoids unrecoverable
failure while still also surfacing the fact that "something" has gone
wrong somewhere.

While doing this, I also fixed a bug where Jiff would perform offset
conflict resolution for `Z` offsets. But it shouldn't do that, since `Z`
reflects an unknown offset. In that case, we always respect the `Z`
offset (as numerically equivalent to `+00:00`) in order to resolve the
instant, and then use the time zone to compute the offset for that
instant unambiguously.

Ref tc39/proposal-canonical-tz#25
Closes #230
I did this in a previous commit when I refactored `TimeZoneDatabase`,
but updating the CHANGELOG in that commit is too annoying. So just do it
here.

Closes #228
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant