-
Notifications
You must be signed in to change notification settings - Fork 42
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
feat: Update Timestamp Constructor with nanoseconds #27
feat: Update Timestamp Constructor with nanoseconds #27
Conversation
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.
There's some value to it.
Could you add tests for this constructor? Since it's now public.
And add dartdocs for it too.
Hello! : ) |
Ah, and I followed the docstring of the code below! :) |
/// | ||
/// Returns a new [Timestamp] representing the same point in time | ||
/// as the given number of microseconds. | ||
factory Timestamp.fromMicros(int microseconds) { |
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.
This one needs a test too
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.
Done! Thanks : )
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.
Is this constructor necessary? It makes sense to stick as close to the JavaScript implementation as possible imo, and I don't believe this exists there (could be wrong)
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.
I believe the Timestamp.fromMicros constructor is necessary. Considering the Dart native environment, the lack of this constructor would result in the loss of microsecond precision.
Additionally, while the microsecondsSinceEpoch property of the Dart DateTime API is available in the web environment, the limitations of JavaScript mean that there will still be a loss of precision in the microsecond unit.
Even though a code like
Timestamp.fromMicros(DateTime.now().microsecondsSinceEpoch);
would not throw an error
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.
Interesting, thanks for the insight. I'd much prefer the name to be fromMicroseconds
, but leave that one to Remi to call :D
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
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 thanks!
Hello!
This update enables preservation of nanoseconds below 1000, addressing the issue where fractions of milliseconds were being discarded.
Thanks!