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

Adding millisecond precision to SnowflakeTimestamp utility #732

Merged
merged 5 commits into from
Jun 17, 2020

Conversation

AndyTempel
Copy link
Contributor

The current utility only allows second precision and maybe to some people millisecond precision will come in handy since Discord timestamps are in milliseconds.

@bwmarrin bwmarrin added the feedback Additional feedback is required label Jan 31, 2020
@CarsonHoffman
Copy link
Collaborator

Instead of introducing the modulus operation and passing both parameters to time.Unix, it might just be easier to pass in timestamp*1000000 to the nanoseconds field and 0 to the seconds field. This is the pattern you'll often see with this function—keeping the parameters mutually exclusive—and takes a bit of load off reading the function (as well as performance with the modulus, as subtle as it may be).

@AndyTempel
Copy link
Contributor Author

AndyTempel commented Feb 8, 2020

Yup, you're correct. I was going by (outdated) benchmarks and didn't test them my self if they still stand.

@ewohltman
Copy link
Contributor

Could this PR be changed to merge into the develop branch of this repo instead of master as per the Contributing guidelines?

@AndyTempel AndyTempel changed the base branch from master to develop March 7, 2020 18:04
@AndyTempel
Copy link
Contributor Author

Branch changed.

@CarsonHoffman CarsonHoffman merged commit 9b1ba78 into bwmarrin:develop Jun 17, 2020
@CarsonHoffman
Copy link
Collaborator

I went ahead and added a quick unit test since we were changing how the function worked (which probably should've been there from the start). Thanks for the PR!

@CarsonHoffman CarsonHoffman removed the feedback Additional feedback is required label Jun 21, 2020
@CarsonHoffman CarsonHoffman added this to the v0.21.0 milestone Jun 21, 2020
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.

4 participants