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

Add SystemTime #9

Closed
wants to merge 3 commits into from
Closed

Add SystemTime #9

wants to merge 3 commits into from

Conversation

expenses
Copy link
Contributor

Hi, It would be super useful to have a SystemTime implementation, mostly for UNIX_EPOCH.

iceiix added a commit to iceiix/stevenarella that referenced this pull request Dec 27, 2020
Although there's an open pull request to the `instant` crate to support
SystemTime and UNIX_EPOCH (sebcrozet/instant#9),
it is simpler to change ui/logo to simply use Instant and Duration.
@wngr
Copy link
Contributor

wngr commented Sep 30, 2021

@sebcrozet I'd be interested in this PR. Would you be open to merging it if I warm it up?

Copy link
Owner

@sebcrozet sebcrozet left a comment

Choose a reason for hiding this comment

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

@wngr Hey! Thank you for the reminder. Yeah, that sounds like a useful PR.

pub struct SystemTime(f64);

impl SystemTime {
pub const UNIX_EPOCH: SystemTime = SystemTime(0.0);
Copy link
Owner

@sebcrozet sebcrozet Oct 1, 2021

Choose a reason for hiding this comment

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

I don’t think this is a valid definition. It is my understanding that doing SystemTime::now().duration_since(SystemTime::UNIX_EPOCH) should return the duration since the unix epoch. However, the now function may return results from the JS function performance.now(), which is not relative to the UNIX epoch.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about wngr@bd46ccb ? If @expenses agrees, I could create a new PR based on her commits.

@sebcrozet
Copy link
Owner

Closing this because #42 was merged instead.

@sebcrozet sebcrozet closed this Oct 18, 2021
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.

3 participants