implement a few deprecations and expand the strtime conversion specifiers #208
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In most cases we just squash the warning instead of cfg'ing out the code. It is just super painful to correctly annotate everything piece of code otherwise.
This applies to `Zoned`, `Timestamp`, `civil::DateTime` and `civil::Date`. Ref #28
Pretty much what it says on the tin. The idea here is that two different `Span` values in memory, which compare not equal, might still be the same duration. Obviously I knew this when I added the trait impl originally, and realized it could be a footgun, but I thought its convenience outweighed the footgun. However, I think #32 pretty convincingly argues that it's the wrong default. If this ends up being the wrong decision, we can always add the trait impl back. In particular, I am worried about this making `Span` a very annoying-to-use type. Not implementing basic equality is just super annoying because it's common to want it in tests and other various things where the footgun isn't really relevant. But at least this way, we can fix the mistake in the future. Ref #32
This does make _some_ use cases a bit more annoying, but I think it overall makes the API more consistent. Essentially, unless `SpanRelativeTo::days_are_24_hours` are provided, then Jiff will always consider days to be a variable unit like weeks, months and years. Previously, this would only happen _some_ of the time. For now, when that assumption is silently made, Jiff will emit a deprecation warning. This will turn into a hard error in `jiff 0.2`. Ref #48
And similarly, deprecate `%:V` in favor of `%:Q`. This was regretably done to improve compatibility with other strtime implementations. In particular, `%V` will, in `jiff 0.2`, correspond to an ISO 8601 week number. This matches other `strftime` and `strptime` implementations, such as GNU's. Ref #147
This adds a whole bunch of conversion specifiers from reading `man strftime` on my system (GNU libc). I originally didn't add most of these because they bloat the size of `BrokenDownTime` and many of them seem a little dubious. But a few folks have filed issues about compatibility. Given the `strtime` API is already a complete clusterfuck, it probably makes sense to just match existing practice as much as we can. One conversion specifier I intend to add but haven't yet is `%V` for the ISO 8601 week number. It is conspicuously absent here since `%V` is currently used for IANA time zone identifiers. That will change in `jiff 0.2`, which will make room for `%V` to be the ISO 8601 week number. Ref #139, Ref #147
BurntSushi
force-pushed
the
ag/variety
branch
from
0628b59 to
a0b0423
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In brief, we:
intzmethods in favor ofin_tz.EqandPartialEqtrait implementations onSpaninfavor of calling
span.fieldwise()to get aSpanFieldwisethat implementsEqandPartialEq.SpanAPIs that days are always 24hours. Instead, callers must either provide a relative reference time or
provide the new special
SpanRelativeTo::days_are_24_hours()marker.%Vin favor of%Qfor IANA time zone identifiers instrftimeand
strptimeAPIs.strftimeandstrptimein near-parity with GNU's implementation. We leave out localespecific specifiers (because Jiff doesn't do locale, use
icufor that)and
%Vfor ISO 8601 week numbers. We'll add%Vfor this injiff 0.2since it's a breaking change. (Hence the aforementioned deprecation.)
See the commit messages and their linked issues for more details.