-
Notifications
You must be signed in to change notification settings - Fork 212
Mostly switch to chrono #781
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
Conversation
This is currently kinda broken, but it's caused me so much pain that I'm hoping someone else knows what the hell they're doing |
This is the relevant error message: https://github.com/rust-lang/docs.rs/runs/718596017#step:11:1876 backtrace
|
Looks like the |
My favorite error, |
Using |
While I agree that using UTC would be preferable, it requires changing every row of the DB that uses |
The database is already using |
The only place I know of to view timestamps is on builds, and you can't serialize a |
9febf27
to
8a9282d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one more thing that could be changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you summarize why we're making this change, including the benefits and drawbacks compared to time
?
src/web/builds.rs
Outdated
"build_time", | ||
&time::at(self.build_time).rfc3339().to_string(), | ||
)?; | ||
state.serialize_field("build_time", &self.build_time.format("%+").to_string())?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does %+
mean? Can you add that as a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%+
just means a RFC 3339 timestamp, it's in the chrono docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a comment anyway in case the docs or behavior ever change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, funnily enough it'll be completely irrelevant once tera finishes, there's no longer a need for a custom serialization routine, but I'll comment it anyways
The reason we're moving to chrono is that it's newer, more widely used and the successor to |
Most of the stuff can be switched to
chrono
, but iron-related timeouts requiretime
and won't be able to be removed untiliron
is.