-
Notifications
You must be signed in to change notification settings - Fork 578
⏰ Add td_*seconds
serde Serialize
and Deserialize
functions for TimeDelta
#1652
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
base: main
Are you sure you want to change the base?
Conversation
td_*seconds
serde Serialize
and Deserialize
functions for TimeDelta
td_*seconds
serde Serialize
and Deserialize
functions for TimeDelta
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'd like to see at least one basic round-tripping test for each of these.
I can't wait for this PR to be merged! |
Thanks for the ping, I totally forgot about this. I'll see if I can get it in a good state somewhere this week. |
f14cedd
to
8432de8
Compare
8432de8
to
9abf81e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1652 +/- ##
==========================================
- Coverage 90.59% 90.26% -0.33%
==========================================
Files 38 38
Lines 16028 16463 +435
==========================================
+ Hits 14520 14860 +340
- Misses 1508 1603 +95 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
8de35ba
to
ce66b33
Compare
I see codecov says no, I haven't really used that before. Does that mean it just requires more tests which touch the added codepaths? |
Fixes #117
This PR copy-pastes the current implementation for
DateTime
such that we have equal implementations forDeltaTime
.I opted to add the logic to the existing
serde.rs
file. However, I think this might be the wrong place and a better solution would be to move themain.rs
'sserde
module combined with thisserde.rs
file to a separatesrc/serde.rs
file, thoughts?PS:
I am just doing a drive-by PR on this crate, so I don't really know what the exact requirements are regarding testing ect. Please let me know!