Display for SystemTime #53892
Display for SystemTime #53892
Conversation
Needed to add another mod in `time`, see following commit.
Time is formatted as ISO-8601 in UTC timezone.
Nanoseconds are printed by default, unless precision is specified:
```
1966-10-31T14:13:20.123456789Z // by default: {}
1966-10-31T14:13:20Z // with format {.0}
1966-10-31T14:13:20.1Z // with format {.1}
```
Implementation is partially copied from `chrono` crate (CC
@quodlibetor).
This is not fully-featured date-time implementation. The motivation
for this commit is to be able to quickly understand what system
time is (e. g. when printed in logs) without introducing external
dependencies.
Format is similar to `java.time.Instant.toString()` output except
that Java truncates trailing zeros by default:
https://repl.it/repls/NauticalSeveralWheel
I think it's better to not truncate zeros by default, because:
* data output (e. g. in logs) look better when field length is the same,
e. g. with default Java formatter this is how log output may look like:
```
1966-10-31T14:13:20.123456789Z Server started
1966-10-31T14:13:21Z Connection accepted
1966-10-31T14:13:21.567Z Request started
```
* precision information is lost (e. g. when reading string
1966-10-31T14:13:20Z it's not clear, is it exactly zero millis, or
timestamp has seconds precision)
This patch depends on leap seconds clarification:
rust-lang#53579
|
r? @bluss (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Can there be some windows specific tests for times that occur before the unix epoch? |
|
@retep007 unit tests in |
|
Big +1 on the premise. Here’s an alternate implementation which is a translation from musl: https://github.com/danburkert/kudu-rs/blob/master/src/timestamp.rs. I don’t have an opinion on which is cleaner or more performant, but thought I’d throw it out there. I don’t claim copyright for that version since it’s translated from musl. |
|
I'd personally prefer that we perhaps have a method on SystemTime similar to We'll want to gather consensus from @rust-lang/libs as well here, I think. |
|
I'd probably prefer we have real |
|
@WiSaGaN one issue I've run into in the past is that |
src/libstd/time/iso_8601.rs
Outdated
| test_impl("5138-11-16T09:46:40Z", 100000000000, 0, 0); | ||
| test_impl("33658-09-27T01:46:41Z", 1000000000001, 0, 0); | ||
| // Largest value GNU date can handle | ||
| test_impl("2147485547-12-31T23:59:59Z", 67768036191676799, 0, 0); |
There was a problem hiding this comment.
According to wikipedia, years more than 4 digits should have a leading +: https://en.wikipedia.org/wiki/ISO_8601#Years. I haven't actually read the standard so I don't know if this is required or recommended or what.
There was a problem hiding this comment.
I think you are right. I've looked in the standard, and it uses a sign everywhere when a year has more than 4 digits. Also, Java implementation adds a sign: https://repl.it/repls/SphericalPrevailingTrapezoids Thank you!
|
@danburkert I am more worried about pulling in the entire date time concept into the standard library without proper vetting just for a convenience method. The |
|
-1 to this PR as-is. This is adding calendaring functionality to the standard library. If we’re going to do this, I think we should also provide a proper API for it (with conversion to a tuple or struct with year/month/etc. components), otherwise people will be tempted to parse the result of |
|
@SimonSapin proper calendar implementation has many features:
I think that all of this is a too large addition to the standard library. It's better to do it in external crate. The goal of this patch is to provide a simple default human-readable display and nothing more.
That is a possibility. Documentation may mention external crates to help people avoiding this mistake. |
|
Updated the patch:
|
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
String parsing would definitely happen, and some of it with bugs. Documentation is not gonna stop that, even when it is read.
Yes. What this means is that this PR is adding a non-proper (incomplete) calendar implementation. And again, if we’re gonna do that, I strongly feel that we should do it with an API that is not only string-based. To express this more formally: @rfcbot postpone |
SimonSapin
added
the
T-libs-api
|
With the appropriate team label… @rfcbot postpone |
|
Team member @SimonSapin has proposed to postpone this. The next step is review by the rest of the tagged teams: Concerns:
Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
rfcbot
added
proposed-final-comment-period
I don't buy this line of reasoning. What's to stop someone from doing the same thing when they import a third party crate to get this behavior? Getting a reasonable human readable representation of |
|
@rfcbot concern i-want-to-merge |
rfcbot
added
proposed-final-comment-period
It is not :) Just to prove it's correct I wrote a simple program which validates random timestamp formatting against Going to submit a fix soon. Edit: done. |
kennytm
added
S-waiting-on-team
There's ISO-8601 standard, and AFAIU it is not available freely. There's a draft available online. It leaves some details to implementation, in particular, how to format years outside of range 0000..=9999: "The interchange parties shall agree the additional number of digits in the time element year" (section 4.1.2.4). And it is not clear if I think the right thing here is to make implementation compatible with Java implementation: print minimum number of digits, but at least 4. Note that JavaScript |
Sorry, I'm saying that I initially believed this was your position. I'm all for adding some basic calendaring functionality to std. |
|
+1 to |
That standard contains a lot of variants like |
|
@SimonSapin RFC 3339 does not allow years outside of range 0000..=9999, which is probably the only ambiguous thing in date representation:
|
|
@stepancheg ah I'm less worried actually about the ISO-8601 format itself and moreso about how we convert and epoch number of seconds to a datetime with y/m/d and such. There's lots of handling there that looks super tricky. Does ISO-8601 cover that sort of conversion though? |
|
@alexcrichton as I said, I've checked implementation is correct with this random program (assuming that AFAIU the tricky part of the calendar implementations is timezone/daylight saving handling. UTC has none of it. Leap seconds handling is another possible complication, and Leap year logic is the same as in ISO-8601, quoting:
Also,
Standard also says that a day is 24 hours, an hour is 60 minutes, and a minute is 60 seconds. This is exactly how the algorithm is implemented. |
|
To weigh in a little bit with my experience handling bug reports for and thinking about the future of chrono. Overall I think that this specific change (adding an alternate UTC display for SystemTime behind some sort of method) will be good and nice. Also, there are lots of complexities around date and time handling, such that even the smallest minimum viable calendar implementation is hugely complex. It took Java several attempts -- the initial release in the 90s barely had java.util.Date, Joda-Time came out initially in 2001, was worked on more-or less-continuously, and was finally standardized in 2014, with significant changes. The most recent attempt in C++ is in the process of getting standardized (according to a comment by the author, I'm not super familiar with the C++ chrono lib) I think that the idea that we will be able to design anything minimal enough to be helpful that will survive half a decade without ugly warts coming up is unlikely. Some points that are just difficult to answer right now are summarized nicely by @gnzlbg when discussing a direct port of C++'s
I think that blocking a well partitioned, UTC- or TAI-based display that is basically rfc-3339 with year extension on a minimal calendar system will mean that you end up with neither. That said, if anyone wants to work with me on breaking up chrono so that we can start seeing what minimal even means for calendars, that might be fun. |
|
We discussed this briefly at today's @rust-lang/libs triage meeting, and @SimonSapin convinced me personally at least that the type returned here could and probably should have accessors for things like year/month/day (of just what we're already displaying). Along those lines though this is getting large enough that I think we may want to have an RFC on this topic instead of just a PR. The |
|
I think Java API (which is based on the famous joda-time library) is close to perfection (it's a bit overengineered, but I'll leave that part). Java API has following classes:
These types are convertible to string (override of There are conversions available between these three main types: What I wanted to say is that: there's no conversion from At that seems perfectly reasonable to me: if a developer needs a year, month and day, then very likely they do some calendaring, and that calendaring is done in a timezone of some user. There are some practical applications when calendars need to be done in UTC, but they are very rare. The only possible practical use case I can think of is formatting a timestamp in UTC timezone but in slightly different format than the default (e. g. space-separated instead of (But if I miss some important use cases, please tell me.) And that rare use case does not justify the existence of a shortcut operation which converts Even full-blown date-time API should not have a no-parameter operation which converts So, sorry, I cannot write an RFC which provides Option 1: write an RFC which simply provides a display Option 2: write a minimal TZ library: This library will be a subset of The library will contain a tz database, that tz database will be a 1Mb symbol in the library, and that symbol should be removed by a linker if And then eventually write an RFC proposing to import that library into |
|
@stepancheg could you comment about how does it support the different calendars ? e.g. C++ |
|
I’d be very hesitant to ship the tz database in the Rust distribution itself. That database is typically updated several times a year and sometimes on very short notice, so tying it to compiler updates doesn’t sound good. (We’re very careful about making compiler updates as painless as possible, but they can still be non-trivial at times. And even a project tracking Stable very closely gets changes 6~12 weeks after they land.) What if fn local() -> TimeZone { ... }I don’t think there should be a no-argument inherent constructor for trying to guess the "local" timezone. (Local to who? Many applications are networked.) Instead the location of this API should more clearly indicate that it is not "pure" computation but might do things like a system call or looking up an environment variable, and behave differently on different machines/environments. In the standard library, this is |
Does any library do this? C++'s |
Ok that sounds more reasonable. But even so, I feel that timezone handling is complex enough that I’d rather not include it at least in the initial set of calendaring APIs we add to the standard library. |
Agreed, I actually would be more comfortable with not including anything in std at all beyond just exposing the system raw time APIs which std already does and to just recommend people to use chrono here :/ |
So, the status quo without this PR? |
|
@SimonSapin pretty much, we tried to kickstart a chrono working group to at some point push chrono to 1.0 but we didn't manage to get enough manpower. The plan was to:
and do all of this while simultaneously still re-exporting everything through the chrono crate to hopefully minimizing breakage. In my opinion, the main problem that creating a good time-date-tz library is a huge amount of work that is hard to prioritize because chrono is actually already "good enough". There are also potentially upcoming language features that would allow for potentially better designs of even chrono-time, e.g., if we had compile-time rational numbers like C++'s ratio, we might be able to build a better chrono-time library, so chrono-date and chrono-tz might end up being better too. These unknowns do not help. All of these "unknowns" make me uncomfortable with trying to make the std time utilities do more than they do now. I'm even unsure if this change makes sense from a structural point-of-view. For example, if I had a crate that only wants to depend on |
|
Yeah, I agree with gnzlbg, their description of chrono is still the plan, and they accurately describe my concerns with the idea of trying to make a minimal calendar API in the standard library. I'm slightly less concerned about adding a UTC display method to SystemTime, but that might just be because I want it all the time. Splitting chrono up so that there are tiny crates for these things should get us to a place where the current situation is less of a concern. The issue is entirely one of working hours at this point -- the experience of Java and C++ over the last several decades mean that we have actual experience to go off of, and so it'd be somewhat reasonable to expect a rust library to get it right... given that there are humans able to put in the time. I do expect to actually have a couple months worth of free weekends for the first time in basically a year, during which one of my highest priorities will be trying to get more contributors to the chrono codebase so that I'm no longer the bottleneck. |
|
I'm closing this PR for now. Thanks for the feedback! |
rfcbot
removed
proposed-final-comment-period
Implement
DisplayforSystemTimeTime is formatted as ISO-8601 in UTC timezone.
Nanoseconds are printed by default, unless precision is specified:
Implementation is partially derived from
chronocrate (CC@quodlibetor).
This is not fully-featured date-time implementation. The motivation
for this commit is to be able to quickly understand what system
time is (e. g. when printed in logs) without introducing external
dependencies.
Format is similar to
java.time.Instant.toString()output exceptthat Java truncates trailing zeros by default:
https://repl.it/repls/NauticalSeveralWheel
I think it's better to not truncate zeros by default, because:
e. g. with default Java formatter this is how log output may look like:
1966-10-31T14:13:20Z it's not clear, is it exactly zero millis, or
timestamp has seconds precision)
This patch depends on leap seconds clarification:
#53579
Issue: #53891