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 from_timestamp_millis to DateTime<Utc> #1374

Merged
merged 2 commits into from
Jan 9, 2024
Merged

Conversation

xmakro
Copy link
Contributor

@xmakro xmakro commented Jan 9, 2024

As discussed in #264 (comment)

Copy link

codecov bot commented Jan 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5536687) 91.68% compared to head (d840420) 91.69%.

Additional details and impacted files
@@            Coverage Diff             @@
##            0.4.x    #1374      +/-   ##
==========================================
+ Coverage   91.68%   91.69%   +0.01%     
==========================================
  Files          38       38              
  Lines       17586    17608      +22     
==========================================
+ Hits        16123    16145      +22     
  Misses       1463     1463              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@djc
Copy link
Contributor

djc commented Jan 9, 2024

Do you have an actual use case for the micros variant?

@xmakro
Copy link
Contributor Author

xmakro commented Jan 9, 2024

I don't need it. I added it consistency but happy to remove it. In my case I serialize DateTime<Utc> with timestamp_millis and then restore it with timestamp_from_millis. If I needed more precision, I might have chosen timestamp_micros.

@djc
Copy link
Contributor

djc commented Jan 9, 2024

Okay, I would prefer to defer the addition of micros in that case until someone comes up with a use case for it. IME millis timestamps are much more common than micros timestamps since they're used a bunch in the JS ecosystem.

@xmakro xmakro changed the title Add from_timestamp_micros/millis to DateTime<Utc> Add from_timestamp_millis to DateTime<Utc> Jan 9, 2024
@xmakro
Copy link
Contributor Author

xmakro commented Jan 9, 2024

Sounds good. I changed the PR to only include timestamp_from_millis.

@djc djc merged commit 6ec8f97 into chronotope:0.4.x Jan 9, 2024
37 checks passed
@djc
Copy link
Contributor

djc commented Jan 9, 2024

Thanks!

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.

None yet

2 participants