time-rs / time

The most used Rust library for date and time handling.
https://time-rs.github.io
Apache License 2.0
1.13k stars 281 forks source link

Generic serialize/deserialize for well-known types #581

Closed CaptainMaso closed 1 year ago

CaptainMaso commented 1 year ago

This pull request allows a user of the time crate to seamlessly use a single well-known serde module with various types, without needing to change the end module that it is pointing at. This is both easier for users, easy to add future impls to support more container types, and easy to add support for more well-known types for all available container types.

Not finished implementation yet, but the current state is:

Should fix #452

Example of use:

#[derive(Serialize,Deserialize)]
pub struct Example {
    #[serde(with = "time::serde::timestamp")]
    time : OffsetDateTime,
    #[serde(with = "time::serde::timestamp")]
    maybe_time : Option<OffsetDateTime>,
    #[serde(with = "time::serde::timestamp")]
    many_times : Vec<OffsetDateTime>
}
codecov[bot] commented 1 year ago

Codecov Report

Merging #581 (8d8e609) into main (e83d01f) will decrease coverage by 0.3%. The diff coverage is 76.7%.

@@           Coverage Diff           @@
##            main    #581     +/-   ##
=======================================
- Coverage   95.7%   95.5%   -0.3%     
=======================================
  Files         79      79             
  Lines       8795    8906    +111     
=======================================
+ Hits        8421    8504     +83     
- Misses       374     402     +28     
Impacted Files Coverage Δ
time/src/serde/mod.rs 84.8% <54.5%> (-11.7%) :arrow_down:
time/src/serde/timestamp.rs 96.2% <95.4%> (-3.8%) :arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

CaptainMaso commented 1 year ago

Is there any interest in this method for handling serde in a nice way? I'm happy to progress this to handle all serde implementations, but if it's not the path to go down I'd rather not spend the time.

jhpratt commented 1 year ago

Looking strictly at the public API (via generated rustdoc), I have some questions and concerns.

CaptainMaso commented 1 year ago
  1. Fair point, I'll take it out.
  2. I think it's worth re-assessing this since the way this approach works doesn't then blow out the module count into time::serde::timestamp::{self,milliseconds}::{option,vec,...}. It's also common enough for web interactions since JS uses milliseconds for time. My initial need for this was that I needed a Vec<OffsetDateTime> from a web form and it was quite involved to make the custom milliseconds -> OffsetDateTime function, then pull in serde-with, wrap it all up and add it. But if the consensus is to leave it out I'm happy to not block the PR on that specific point.
  3. Yep, it's included in the PR: [T] can implement AsWellKnown but not FromWellKnown.
  4. I like that idea, if you could point me to an example of how to do this, I'd much appreciate it!

Thanks for the review!

jhpratt commented 1 year ago

For how to limit the ability to access something:

mod private {
    pub struct Timestamp;
    pub trait AsWellKnown {}
}

use private::Timestamp;
use private::AsWellKnown;

So long as there's never a pub use, the items remain inaccessible from other crates despite being public. This is because any use would have to include the private module.

jhpratt commented 1 year ago

Would you mind providing an update?

jhpratt commented 1 year ago

Closing as inactive.