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

remove unused time features #43

Merged
merged 1 commit into from
Mar 1, 2022
Merged

Conversation

dimbleby
Copy link
Contributor

@dimbleby dimbleby commented Feb 27, 2022

leaving out std doesn't really do anything because formatting implies std per https://docs.rs/crate/time/0.3.0/features.

However local-offset is unused here, and since this is the feature that reintroduces thread-safety shenanigans it seems worthwhile to avoid it if we can.

It may be that what this really means is that the current implementation of timestamp_local() is in fact not doing anything to localise the timestamp, and that's a bug. I'm in the UTC timezone, so it's not quite straightforward for me to test this!

Edit: yeah, I'm pretty sure that there is no localisation going on. Probably what you really want is

-    let now: time::OffsetDateTime = std::time::SystemTime::now().into();
+    let now = time::OffsetDateTime::now_local();

the implementation of time::OffsetDateTime::now_utc() is exactly what you currently have in timestamp_local()

I'll submit a second MR and you can choose!

@dimbleby dimbleby mentioned this pull request Feb 27, 2022
@Techcable
Copy link
Member

Wow 👀

Good investigative work.....

@dpc dpc requested a review from Techcable March 1, 2022 20:36
Copy link
Member

@Techcable Techcable left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine to merge right now. I don't think there is any risk....

I want to look at #44 more closely because

  1. It triggers an "indeterminate offset" error both on my laptop and on Github
  2. It addresses a very serious bug
    • However because of the error, I am not sure the solution works

@Techcable Techcable merged commit 8c90668 into slog-rs:master Mar 1, 2022
@dimbleby dimbleby deleted the time-features branch March 2, 2022 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants