Skip to content

Dylan/feat: Static TimeConvert util #35

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dylanh724
Copy link
Contributor

@dylanh724 dylanh724 commented Apr 25, 2024

About

Add a static TimeConvert helper class to the Unity SDK

Why?

In Rust, .timestamp() is pretty straight-forward. However, for C#? There are a million ways to do it, and almost all of the ones most people would initially think are probably wrong (including my own theories when I was eating my own dogfood converting scripts from rs to c#).

I was originally using an ISO timestamp since it's accepted by most DBs, for example - that was not in microseconds. However, it can also get messy quite fast and can inaccurate as things get parsed back-and-forth.

To simplify things, this helper class can help keep things consistent and makes it as close as the Rust timestamp() call as possible, converting both ways.

EDIT: After Ingvar pointing out there's an official DateTime.UnixEpoch, this makes this PR way less needed. However, it may still be useful to provide official convert consistency (with inline docs) since DateTime-like converts always raise questions/concerns.

Feats

  • public static DateTimeOffset FromMicrosecondsTimestamp(long microseconds)
  • public static long ToMicrosecondsTimestamp(DateTimeOffset dateTimeOffset)
  • public static long MicrosecondsTimestamp()
  • [Obsolete] public static string ToTimestampIso8601(DateTimeOffset dateTimeOffset)
    • Kept in case you need to transfer the data somewhere else, where often an ISO 8601 timestamp string is required. There's a hefty note warning that this is not as accurate, recommending another func.

Additional Note

I recently updated Zeke's Demo mini upgrade PR to have a duplicate TimeConvert class to ensure synchronicity (which will dupe into the new demo docs later).

/// </summary>
public static class TimeConvert
{
private static readonly DateTimeOffset unixEpoch = new(
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL! I'll adjust this

Copy link
Contributor Author

@dylanh724 dylanh724 Apr 29, 2024

Choose a reason for hiding this comment

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

@RReverser After editing this out, there's not much left! It may be still worth it just to provide an "official" way to convert back and forth (with some inline docs to explain better) since there are always questions about the best practices for time converts.

This is especially emphasized since I personally got it wrong when using SpacetimeDB by using ISO timestamp strings at first, not realizing it doesn't get as low as microsecs (in addition to long being more efficient than string).

Comment on lines +32 to +36
public static long MicrosecondsTimestamp()
{
TimeSpan elapsed = DateTimeOffset.Now - DateTime.UnixEpoch;
return elapsed.Ticks / 10;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Still not sure about the class itself and going to leave it to others to review, but another small nit is that we can reuse the more general method here:

Suggested change
public static long MicrosecondsTimestamp()
{
TimeSpan elapsed = DateTimeOffset.Now - DateTime.UnixEpoch;
return elapsed.Ticks / 10;
}
public static long MicrosecondsTimestamp() => ToMicrosecondsTimestamp(DateTimeOffset.Now);

@jdetter jdetter removed their request for review November 8, 2024 18:10
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