time icon indicating copy to clipboard operation
time copied to clipboard

Breaking change wish-list

Open jhpratt opened this issue 1 year ago • 13 comments

Note: There is no breaking change current planned. This issue is to keep track of things that may happen when there is a breaking change at some undetermined point in the future. Items are in no particular order.


Potential changes

These are mostly things that I have thought about at some point, with varying levels of certainty.

  • Eliminate large-dates feature flag. Support the ±999,999 range of years unconditionally, adding a modifier to the [year] component to solve the issue of ambiguity.
  • Remove the serde-well-known feature flag, which is already deprecated in favor of using the relevant flags (serde, formatting, and/or parsing) directly.
  • Simplify error handling. Anonymous enums would be wonderful but are not required. In particular, the large number of conversions should be cut down, as many of them exist when they can already be performed transitively. The amount of information exposed should be restricted to as little as necessary, as this has caused issues in the past.
  • Remove anything deprecated. This is currently minimal.
  • Make FormatItem opaque and require FormatItem::Literal to be valid UTF-8. Likewise for OwnedFormatItem.
  • Remove Copy implementation for Parsed.
  • Use ranged integers in public APIs. This would currently require adding countless new methods.
  • Rename Duration to SignedDuration or something else.
  • Make Duration::seconds generic over i64, f32, and f64. Similarly for saturaturating_seconds_* and checked_seconds_*. Alternatively, have a seconds_float method that is generic while keeping the integer case separate.
  • Remove Duration::time_fn. This is a carry-over from time 0.1 and was presumably meant as a poor man's benchmarking tool. Instant is not meant to be used in that manner.
  • Remove time::Instant in favor of an extension trait on std::time::Instant. This would reduce the number of trait implementations needed. The extension trait was added in v0.3.35, with time::Instant being deprecated at the same time.
  • Semantically permit modifiers on [optional] and [first] (needed for #708)

jhpratt avatar Jan 29 '24 03:01 jhpratt

Separate feature flags for serde and the provided impl Serialize/Deserialize for T would be very helpful (and address https://github.com/time-rs/time/issues/672)

jeff-hiner avatar Mar 29 '24 16:03 jeff-hiner

@jeff-hiner I'll be honest, it's not likely to happen. The clippy issue you linked would solve your request while being more general purpose and would avoid the overhead of an additional feature flag. I'm also not aware of any widely-used crates that gate serde support and Serialize/Deserialize impls separately.

jhpratt avatar Mar 31 '24 16:03 jhpratt

I'm also not aware of any widely-used crates that gate serde support and Serialize/Deserialize impls separately.

The url crate (167M downloads) implement the serde traits with a sensible representation (the string representation one expects), and then provides serialize_internal and deserialize_internal for when you want to use a more performant serialization format.

To me the situation here is similar to Path not implementing Display, but still providing the Path::display method that returns something that implements Display, because providing the implementation directly would be a footgun.

nox avatar Apr 02 '24 16:04 nox

Note that I was responding to the use of separate feature flags for Serialize/Deserialize impls, which is not present in url. Regardless, time also uses a sensible representation for implementing serde traits — the same format as is used for Display. Just as serialize_internal and deserialize_internal necessitate the use of serde helper attributes, you can already use helper attributes provided by time on the relevant fields. The only concern I have seen raised is relating to the default not being a well-known format, but that can be worked around if and when that clippy issue is resolved. If it's that much of an issue for you, I suggest you bring it up with your (common) employer to find some time to work on it.

Path not implementing Display is not because it's a footgun but because it's a lossy conversion. That is not the case for any type in time.

jhpratt avatar Apr 03 '24 03:04 jhpratt

Regardless, time also uses a sensible representation for implementing serde traits — the same format as is used for Display.

That's false. You find in the string printed by the Display the same amount of information as in the tuple the serde impl serializes to, but the serde impl doesn't serializes as a string. With this logic, you could argue that url's serialize_internal uses the same format as Display, and that would be wrong.

Oh with the human-readable serializer you mean? That's fair.

nox avatar Apr 03 '24 08:04 nox

@jeff-hiner I'll be honest, it's not likely to happen. The clippy issue you linked would solve your request while being more general purpose and would avoid the overhead of an additional feature flag.

Looks like the clippy issue has been parked for 2 years. It was claimed, but I don't see any progress over the last year. I'd try to PR in a fix myself but as it involves digging into clippy's parser I honestly have no idea where to even start. Not optimistic that's going to be fixed soon.

I'm guessing here, but it looks like the current default is intended for human-readable serialization like serde_yaml. I suppose the similarity with Display is convenient for that use case. Unfortunately this default is unsuitable for those of us using serde_json and time to write REST endpoints or API clients, because it doesn't appear to follow any RFC or ISO convention.

I don't mind having to explicitly annotate serde(with = "..."). But the compiler can't help me if I miss one, because there's a valid but incompatible impl in scope. chrono::DateTime<Utc> defaults to RFC3339, which is why I'm only noticing it on my migration.

Since there's no one "right" way to impl Serialize for OffsetDateTime, maybe we simply shouldn't provide one. Removing the default Serialize/Deserialize entirely and making users opt into a specific serde impl set seems like the only sensible compromise. This would be a breaking change, thus why I'm bringing it up here.

jeff-hiner avatar Apr 03 '24 19:04 jeff-hiner

It was claimed, but I don't see any progress over the last year.

Are we looking at the same thing? There was an update just a couple days ago on a PR that implements it. It may have stalled, but it is at least on the person's backlog.

it looks like the current default is intended for human-readable serialization

The default is intended to be something human-readable (when that flag is enabled) that can also be deserialized. There was no reasoning beyond that.

Unfortunately this default is unsuitable for those of us using serde_json and time to write REST endpoints or API clients, because it doesn't appear to follow any RFC or ISO convention.

chrono::DateTime<Utc> defaults to RFC3339

A trivial check shows that chrono does not use RFC3339, as that specification does not support years that are negative or contain more than four digits.

Since there's no one "right" way to impl Serialize for OffsetDateTime, maybe we simply shouldn't provide one.

That's an awfully large leap. As I said, the intent was to have something that is human-readable and can be round-tripped. Just as with formatting being present because Display is not everyone's cup of soup, there is a way to override the default. That there is not currently a way to prevent you from using the default does not mean we should take something reasonable away from those who want it. It's an ecosystem problem, not a time problem imo.

jhpratt avatar Apr 04 '24 05:04 jhpratt

It was claimed, but I don't see any progress over the last year.

Are we looking at the same thing? There was an update just a couple days ago on a PR that implements it. It may have stalled, but it is at least on the person's backlog.

I'm looking at https://github.com/rust-lang/rust-clippy/issues/8581 as linked in my original comment. That issue was opened 24 Mar 2022 (over two years ago), then claimed in June of the same year. A separate author opened a PR that was last touched at the beginning of the year, in Jan 2024. That PR received feedback in Feb, but no commits have been made since. As of right now the PR has conflicts. It appears the work required to have clippy support the appropriate parsing is non-trivial, or at the very least the PR implementation is controversial.

A trivial check shows that chrono does not use RFC3339, as that specification does not support years that are negative or contain more than four digits.

A trivial check on docs.rs shows that chrono does in fact use RFC3339 for DateTime<Tz>. It's very possible the implementation isn't complete or correct, but RFC3339 appears to be the intent of the default serialization.

jeff-hiner avatar Apr 05 '24 21:04 jeff-hiner

Things aren't fast in Rust given that it's entirely driven by volunteers. The PR author commented just this week that they intend to continue work on it. Given that you presumably are using this as part of your employment, that is why I suggested asking for some time to work on the issue.

It's very possible the implementation isn't complete or correct, but RFC3339 appears to be the intent of the default serialization.

It definitely isn't correct for the reason that I stated. It's not RFC3339 if it doesn't strictly follow the specification.

jhpratt avatar Apr 05 '24 22:04 jhpratt

Regardless of the specific format chosen, a breaking change I would really like is for the time crate to adopt some standard datetime representation as the default or simply get rid of the default Serialize/Deserialize impls. I would vote for ISO 8601, which avoids the RFC 3339 restriction on negative years.

The current default Serialize impl is really frustrating because the format appears to be bespoke and is thus only meaningfully compatible with time itself.

FeldrinH avatar Jun 20 '24 14:06 FeldrinH

Removing the defaults is almost certainly not going to happen. ISO 8601 is at least a possibility, though keeping the non-human-readable compatibility will almost certainly happen.

thus only meaningfully compatible with time itself.

That's pretty much the point, actually. It's meant for when someone wants to serialize it for storage/transmission and then deserialize it at the other end. It is not and was never intended to be general-purpose — that's what the attributes are for.

jhpratt avatar Jun 21 '24 08:06 jhpratt

I don't really mind the non-human readable variant, since as far as I know there is no widely used binary encoding standard for dates and times. But I think replacing the default human readable representation with a standardized one has only benefits: it avoids the work of maintaining an extra format and interop with other systems is easier out of the box.

FeldrinH avatar Jun 21 '24 11:06 FeldrinH

For what it's worth, "maintaining" the extra format is essentially zero work, as it was a one-off thing that hasn't changed in the slightest. Changing it to a different format wouldn't be much work, and looking at the current implementation it's not even that different from ISO 8601.

jhpratt avatar Jun 21 '24 23:06 jhpratt