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

Time v0.2 pre-release feedback #190

Open
jhpratt opened this issue Nov 21, 2019 · 25 comments
Open

Time v0.2 pre-release feedback #190

jhpratt opened this issue Nov 21, 2019 · 25 comments
Labels

Comments

@jhpratt
Copy link
Member

@jhpratt jhpratt commented Nov 21, 2019

Hello rustaceans! Until recently, the time crate was soft-deprecated, in that bug fixes would be considered, but no features would be actively added. It is the 25th most downloaded crate all-time, and is still being downloaded slightly more than chrono. As such, I began a bottom-up rewrite of the time crate, taking inspiration from time v0.1, chrono, the standard library, and some RFCs, along with some localization pulled in from glibc.

My goal is to release time v0.2 alongside Rust 1.40, which is currently scheduled to be released on December 19. By going to the community for a public review now, this will allow me sufficient time to make any necessary changes.

To view the documentation for the master branch, you can use time-rs.github.io. There are also some helpful shortcuts, so you can go to time-rs.github.io/Duration and be brought to the relevant documentation immediately.

Follows are two pages from the wiki, which provide some detail as to what will occur in the future and the differences between existing crates.


Differences from chrono and time v0.1

Chrono has been the de facto crate for time management for quite a while. Like time, its original implementation predates rust 1.0. Presumably due to its age, chrono still relies on time v0.1 (though it's partially inlined the relevant code). The disadvantage of this is that any third-party functions that rely on chrono essentially mandate you import chrono or time v0.1 to use its Duration type. This single fact makes interoperability with the standard library quite difficult. Time v0.1 is identical to chrono in this manner.

Time v0.1 is what the standard library was built upon. Time v0.2 flips this, being built upon the standard library. This provides for a higher-level abstraction, while also supporting the same targets as the standard library.

In time v0.2, there is full interoperability with the standard library. Every type can perform the same arithmetic that its standard library counterpart can and vice versa. Types are freely convertible to and from their standard library equivalents.

Vision

Scope

Most things that simplify handling of dates and times are in the scope of the time crate.

Some things are specifically not in scope.

  • Nothing should implicitly rely on the system's time zone. Passing a time zone as a parameter or a function returning the time zone is acceptable. This restriction ensures that nothing surprising happens when running the same code on different systems.

Explicitly in scope, but not yet implemented include the following:

  • Full timezone support, relying on tzdb.
  • Period type, which would be usable with a zoned DateTime. While this would expose a similar (if not identical) API to Duration, the advantage is that it could take into account leap seconds, DST, etc. As such, a Period would not be directly convertible to a Duration - one minute could be either 59, 60, or 61 seconds.

Both lists are explicitly non-exhaustive. Additional items that are not in scope will be added to this list as they are brought up.

Strategy

Wherever possible, types are implemented as wrappers around those in the standard library. Additional functionality is still able to be provided, such as allowing a Duration to be negative.

Some types (like Sign) were helpful to implement generically, simplifying arithmetic implementations. They may be extracted to a separate crate in the future and re-exported in time.

Future plans

If all necessary reviews go well, my intent is to release time v0.2 shortly after the release of rust 1.40.

For post-v0.2 features, eventually I'd like to support tzdb, including automatic time zone shifts for DST and native leap second support. I'd also like to be able to have 5.seconds() be able to return either a time::Duration or std::time::Duration, but the compiler is currently unable to infer the return type and choose the appropriate trait to use.


Let me know your thoughts, positive or negative. All I ask is that you be constructive and follow the code of conduct. I can be reached on Matrix either via direct message or on the #time-rs channel.

@jhpratt jhpratt pinned this issue Nov 21, 2019
@wezm

This comment has been minimized.

Copy link

@wezm wezm commented Nov 21, 2019

Some questions that come to mind after reading this issue and poking through the docs and code briefly:

  • Is this crate compatible with 0.1?
    • If not, maybe the deprecated things should be omitted?
  • What's the thought behind using log to emit the deprecation warnings?
    • Seems people are unlikely to see these messages and the parent functions will emit a deprecation warning at compile time anyway. I guess this mostly seems like an opportunity to avoid bringing in the log crate in the default set of feature flags.
@jstrong-tios

This comment has been minimized.

Copy link

@jstrong-tios jstrong-tios commented Nov 21, 2019

my wish list item: a u64 wrapper representing a nanosecond timestamp with serialization/deserialization and interoperability with the other types. it's not always possible to use u64 to store timestamps (e.g. dates > 2554-07-21) but in many cases it is, and u64 is half the size of a libc timespec. (i64 would be ok as well, max date is 2262-04-11). relatedly, a i32/u32 wrapper representing hourly precision timestamps, u16 representing daily, etc. I've used these approaches in certain circumstances but it would be a joy to have a library that allows painless conversion between these and more full-fledged datetime types.

@jhpratt

This comment has been minimized.

Copy link
Member Author

@jhpratt jhpratt commented Nov 21, 2019

@wezm

Some questions that come to mind after reading this issue and poking through the docs and code briefly:

  • Is this crate compatible with 0.1?

    • If not, maybe the deprecated things should be omitted?
  • What's the thought behind using log to emit the deprecation warnings?

    • Seems people are unlikely to see these messages and the parent functions will emit a deprecation warning at compile time anyway. I guess this mostly seems like an opportunity to avoid bringing in the log crate in the default set of feature flags.
  1. No, it is not fully compatible with 0.1. Some methods and types are included (and deprecated) where names and/or signatures have changed. Without looking at the full API, I believe most of the Duration and Instant types are compatible. As they're deprecated, they shouldn't be used in new code.

    I've also received a direct message on Matrix asking about an equivalent of the time::at method from 0.1. The equivalent would be DateTime::from_unix_timestamp(seconds) + nanos.nanoseconds(). Because it's relatively simple, I'll likely push the additional deprecated method (and similar methods) in a bit for additional back-compatibility. After looking at the old API, these functions return Tm and Timespec structs, which no longer exist.

  2. Strictly speaking, it's not a deprecation warning. It's a warning against a slight change in behavior that I felt was favorable to panicking. Users might not necessarily run across the specific situation (overflowing). It's not a bad idea to just add it to the built-in deprecation warning, or even revert to the old behavior of panicking. Either of those would eliminate the default dependency on log.


@jstrong-tios

my wish list item: a u64 wrapper representing a nanosecond timestamp with serialization/deserialization and interoperability with the other types. it's not always possible to use u64 to store timestamps (e.g. dates > 2554-07-21) but in many cases it is, and u64 is half the size of a libc timespec. (i64 would be ok as well, max date is 2262-04-11). relatedly, a i32/u32 wrapper representing hourly precision timestamps, u16 representing daily, etc. I've used these approaches in certain circumstances but it would be a joy to have a library that allows painless conversion between these and more full-fledged datetime types.

The primary problem with doing something like this is that the epoch is not immediately obvious. You could always do something like DateTime::unix_epoch() + 1_000.days() +. To go the other direction, you could do (datetime - DateTime::unix_epoch()).whole_days().

@jstrong-tios

This comment has been minimized.

Copy link

@jstrong-tios jstrong-tios commented Nov 21, 2019

@jhpratt do you mean it wouldn't be obvious whether the type uses 1970-01-01 or some other epoch (1582! who's with me!?) as its reference point? My assumption was it would use the unix epoch; I think that would be the obvious choice. But there's a variety of ways to mitigate possible confusion, including, as you suggest, explicitly named "constructor" methods. In uuid crate, I was involved in improving the api for their v1 timestamp functionality which used the same idea: https://docs.rs/uuid/0.8.1/uuid/v1/struct.Timestamp.html. At a deeper level, I think that someone who cares about the memory layout of their timestamps can be trusted to read the docs specifying the epoch.

@jhpratt

This comment has been minimized.

Copy link
Member Author

@jhpratt jhpratt commented Nov 21, 2019

@jstrong-tios Yes, that is what I meant with regard to the epoch. I would personally assume the unix epoch as well, but I don't think that should be the case without being clear in the method name. That would be similar to other items excluded in the scope, where any assumptions are clearly indicated.

However, you do make a decent argument with the size of the struct. Running std::mem::size_of::<T>() gives the following currently:

Size (bytes) Struct
8 Date
16 DateTime
16 Instant
20 OffsetDateTime
0 OutOfRangeError
8 Time
4 UtcOffset

On initial thought, I could do something along the lines of Week, Day,Nanosecond, implemented as thin wrappers around the appropriate integer type. However, I'd only permit conversions to Durations, not to DateTimes directly (due to the epoch ambiguity). They would likely be placed into a submodule, though I'm not sure on the name. period would conflict with the future Period type, which won't be directly convertible to a Duration.

Feel free to join the Matrix channel if you'd like to discuss this outside of this issue! I'll likely respond quicker there as well.

@aloucks

This comment has been minimized.

Copy link

@aloucks aloucks commented Nov 22, 2019

Why not name the functions the same as their std library counterparts?

For example:

as_secs_f32
vs
as_seconds_f32

I think there would be value in having this as a drop-in replacement.

@jhpratt

This comment has been minimized.

Copy link
Member Author

@jhpratt jhpratt commented Nov 22, 2019

@aloucks I presume you're only referring to Duration here, as the only other type with an identically-named counterpart is Instant, where I used the same API.

Basically what it comes down to is I think that "secs" is odd to use — some others like millis, micros, and nanos are more common, but as a native English speaker, I've never seen anyone use "secs" when referring to seconds in any context other than that one. Plus, at what point do you stop shortening names? Why not just Duration::as_s()?

While I understand the concern, I think it's more logical to just spell it out. Time wouldn't be able to be a drop-in replacement regardless, due to differing behaviors and function headers due to its ability to be negative. And on top of that, you picked probably the most similar method name 😛 Most of the methods are whole_minutes and similar, which provide a more descriptive name of what is returned.

Side note! If Rust had method overloading, my ideal API would actually be along the following lines:

impl Duration {
    fn seconds(i64) -> Self; // same as current
    fn seconds(self) -> i64; // same as `.whole_seconds()`
    fn seconds(self) -> f32; // same as `.as_seconds_f32()`
    fn seconds(self) -> f64; // same as `.as_seconds_f64()`
}

Unfortunately that's not quite the case. I looked into just supporting .as_seconds() in a trait and trying to infer whether it was f32 or f64, but alas that's not even currently possible.

@jhpratt

This comment has been minimized.

Copy link
Member Author

@jhpratt jhpratt commented Nov 22, 2019

@jstrong-tios Looking into this some. It looks doable, though I'm not 100% sold on it being in the 0.2 release. There would be 8 types and conversions in between all of them. Then we'd also have arithmetic between the various types (adding, subtracting, and dividing, plus multiplying by integers), equality, ordering, etc. Needless to say, there would likely be a few hundred manual trait impls that could only be partially generated with macros.

@jstrong-tios

This comment has been minimized.

Copy link

@jstrong-tios jstrong-tios commented Nov 22, 2019

@jhpratt if it's any use to you, I just dug up this implementation of a daily and hourly integer wrapper types I wrote for an unpublished project. What do you have in mind in terms of things that cannot be done with macros? Comparing between the types?

@jhpratt

This comment has been minimized.

Copy link
Member Author

@jhpratt jhpratt commented Nov 22, 2019

Yeah, that's roughly what I had in mind for the API, sans chrono of course.

With regard to macros, off the top of my head I think a large portion wouldn't be able to due to the conversion constants. I believe there's a way around it, but I'd have to check to be sure (it deals with the orphan rule). I could, of course, convert it to a Duration and then to the target type, but that defeats the goal of the smaller memory footprint.

@KodrAus

This comment has been minimized.

Copy link
Collaborator

@KodrAus KodrAus commented Nov 24, 2019

I’m a little unsure about trying to bake in our own i18n effort here. Specifically whether it’s likely to hinder broader support in the future for apps that need to work with more than just dates and might run into inconsistencies with libs that take different approaches. Do you know of any other time-like libraries in other language ecosystems that follow this path?

@jhpratt

This comment has been minimized.

Copy link
Member Author

@jhpratt jhpratt commented Nov 24, 2019

Not entirely sure what you mean by potential inconsistencies with other libraries. I suppose there could be differences in spellings or abbreviations if that's what you mean, but I'm not aware of any major i18n/l10n crate in the ecosystem. Upon a quick search, i18n is reserved and l10n is available, so I could spin it into a separate crate (with wider support) and depend on that if that would be preferred.

With regard to other languages, yes! I actually pulled the localization from glibc.

  • glibc uses the "current" locale, so it's not possible to manually set it (aside from changing environment variables).
  • Python and Ruby, using the "current" locale.
  • JavaScript has Intl.DateTimeFormat, which uses full locales.
  • JodaTime in Java has partial support for locales (ctrl+F for "locale" on that page). However, JodaTime was largely absorbed into a core Java API (notably not the locale support, though).
  • Swift supports arbitrary locales

Of those that I searched, PHP, C♯, and Go appear to have neither language nor locale support (PHP says so explicitly). The others are above. So at least from first glance, it seems a significant number of languages have some level of support, even if not customizable.

I think it's important to have it not implicitly rely on the system locale or language, as that causes inconsistencies when running otherwise identical systems. At the minimum, the user should have to call Language::system() (or something along that line). Having support in general is useful for anyone doing l10n on web servers or client-facing software.

@jhpratt

This comment has been minimized.

Copy link
Member Author

@jhpratt jhpratt commented Nov 25, 2019

@wezm I've just removed the log dependency in favor of a more detailed compile-time message. time::precise_time_ns() and time::precise_time_s() have also been added & deprecated.

@jstrong-tios After thinking about your idea a bit more, I think it would be best to push this past 0.2, given the inherently large amount of code & complexity.

@jhpratt

This comment has been minimized.

Copy link
Member Author

@jhpratt jhpratt commented Nov 29, 2019

@KodrAus Would you prefer 0.2 not have localization? Everything could be in English, which is currently assumed if not specified. It would be relatively easy to remove the languages as it currently is.

Looking down the road, I recently ran across the Unicode Common Locale Data Repository, which seems to be the de facto authoritative source on locale information, being used by Windows, Mac, and most Linux distros, as well as by browsers like Chrome and Firefox. I am going to look through the raw data some more; I'm trying to determine how hard it would be to extract the requisite information and publish a l10n crate, which time could then depend upon. That would be a ways away, though.

@KodrAus

This comment has been minimized.

Copy link
Collaborator

@KodrAus KodrAus commented Nov 29, 2019

Thanks for your patience on this @jhpratt! And I should’ve mentioned before that the new API you’ve designed is great and a real leap forward. I’m trying to start at a higher-level before digging into specific API decisions.

Not entirely sure what you mean by potential inconsistencies with other libraries. I suppose there could be differences in spellings or abbreviations if that's what you mean, but I'm not aware of any major i18n/l10n crate in the ecosystem.

Would you prefer 0.2 not have localization? Everything could be in English, which is currently assumed if not specified. It would be relatively easy to remove the languages as it currently is.

I haven’t actually worked on a localized app myself, so this is really just my impression based on the scope of the localization API. If anybody who has needed to work in this space has different impressions I would defer to those 🙂

I imagine a project that requires localization would want a fully-featured localization system like Project Fluent, which looks like it has out-of-the-box datetime support, and an implementation for Rust. In this case we don’t need to build localization support into time, and consumers don’t need to work with potentially inconsistent localization APIs from different libraries in their project when supporting different cultures.

@jhpratt

This comment has been minimized.

Copy link
Member Author

@jhpratt jhpratt commented Nov 30, 2019

No problem @KodrAus. I haven't actually done a large-scale app that requires localization (yet), so I'd likewise defer as well if anyone else is following this and has input.

Fluent seems useful, and handles both full and abbreviated text as well; it appears to be based off of JavaScript's API. You're probably right that anything needing localization would be on a scale higher than just dates and times.

After taking a quick look at Fluent on the playground, I'll almost certainly pull the localization code tonight, unless I see some significant reason not to.

Update: Language handling has been fully removed.

@Y0ba

This comment has been minimized.

Copy link

@Y0ba Y0ba commented Dec 2, 2019

Currently functions which create Date/Time objects from parts (e.g. Date::from_ymd) panic if you supply wrong parts. Is it possible to make them return Result, or add another methods to return Result, so i can track these errors?

@jhpratt

This comment has been minimized.

Copy link
Member Author

@jhpratt jhpratt commented Dec 2, 2019

@Y0ba I can look into additional methods that would return an Option or Result, though the latter may be slightly more complex (not too bad, though). What do you think they should be called? Date::from_ymd_opt and similar? The main thing I'll have to look into is how it'll interact with the macro for checking the value is in range.

@Y0ba

This comment has been minimized.

Copy link

@Y0ba Y0ba commented Dec 2, 2019

My usecase is that these parts come from device via COM-port, and they might be either wrong or just corrupted, therefore both Option and Result are fine by me since I don't need exact reason, just the fact of failure (but maybe someone else will need it for better logging or error reporting 🤷‍♂).

What do you think they should be called?

_res or _opt suffixes are fine. _fallible looks better but it's probably too long.

@jhpratt

This comment has been minimized.

Copy link
Member Author

@jhpratt jhpratt commented Dec 2, 2019

Ok, sounds good! I'll take a look into it tomorrow or Tuesday; hopefully updating the macros won't be too tricky.

@jhpratt

This comment has been minimized.

Copy link
Member Author

@jhpratt jhpratt commented Dec 3, 2019

Well, I just tried implementing a trait and a variety of objects to allow for a very detailed error (actual range, given value, what type it was), but it seems like it's not currently possible to do without a ton of effort. I could likely work around it by creating another layer of abstraction, but then it gets confusing to follow for the end user.

So, they'll be methods returning an Option. And the name in the form of try_foo, which is a bit more descriptive.

Update: Implemented returning an Option! @Y0ba

@jhpratt

This comment has been minimized.

Copy link
Member Author

@jhpratt jhpratt commented Dec 4, 2019

Issue I would like to resolve: std::time::Duration allows for values between 0 and u64::max_value(); time::Duration between -u64::max_value() and u64::max_value().

Should Duration::whole_seconds() return an i128 rather than an i64? That would allow the full range to be captured if someone did Duration::seconds(i64::max_value()) * 2 (which results in a valid value). Right now, whole_seconds() overflows. I believe the alternative is to forcibly restrict the range, which should be doable without too much effort.

I can't think of any conceivable reason why i64::max_value() seconds (positive or negative) wouldn't be sufficient for any use case; it's literally longer than the age of the universe.

How overflows are handled in general is something that I do need to check and possibly modify behavior before a release, though.


Update: Overflow handling should be sensible now. max_value and min_value have been undeprecated, and the range of Duration is restricted.

@stevenroose

This comment has been minimized.

Copy link

@stevenroose stevenroose commented Dec 9, 2019

Are there any good reasons to require Rust v1.40? I'm quite curious. It seems like a time crate should be able to be built using older compilers.
We're currently using v1.22, v1.32 and v1.36 because of different reasons/constraints. I realize that 1.22 is very old, especially in Rust land, but it should be possible to at least target a version that has been stable for a few months instead of a version that is not even released yet..

@jhpratt

This comment has been minimized.

Copy link
Member Author

@jhpratt jhpratt commented Dec 9, 2019

@stevenroose #[non_exhaustive] is used, which is currently in beta. The alternative is to have a __nonexhaustive variant on the relevant enums, which requires explicit panics inside the library where it's used.

After the initial release, bumping the minimum version for rustc will allow for the then-current stable and the two previous versions (which would currently be 1.37, as stable is 1.39). I don't think I wrote that down anywhere, so I'll add it to the wiki in a bit.

Keep in mind that once rust-lang/rust#65262 lands, that stable can be the permanent MSRV, as any future changes could be gated behind that. This is currently possible via rustversion crate, but that would require adding in a mandatory proc macro dependency, increasing compilation time. If I implement #191 using proc_macro_hack instead of waiting for nightly APIs to stabilize, I might consider using rustversion as well.

@jhpratt

This comment has been minimized.

Copy link
Member Author

@jhpratt jhpratt commented Dec 10, 2019

@alexcrichton @KodrAus Any final concerns, or can publishing rights be granted on crates.io in anticipation of a release next Thursday(ish)? That's the final item in the rough plan here from back in September.

Pinging @Manishearth as well, who had initially expressed reservations about transferring control of the crate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.