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

Generic serialize/deserialize for well-known types #581

Closed
wants to merge 5 commits into from

Conversation

CaptainMaso
Copy link

@CaptainMaso CaptainMaso commented May 18, 2023

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:

  • Added timestamp::millis for a millisecond resolution unix timestamp.
  • Added AsWellKnown and FromWellKnown traits to handle the parsing between types.
  • Implemented AsWellKnown and FromWellKnown for container types: Option, Vec
  • Implemented AsWellKnown for [T] types, which should allow for serialising Box<[T]> style, as they should deref into [T]. I haven't tested this yet.
  • Implemented AsWellKnown and FromWellKnown for timestamp and timestamp millis.
  • I've also expanded the tests for timestamp to include timestamp millis.

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>
}

 - Allows serde::timestamp to be used for more common types without
   additional modules.
 - Added serde::timestamp::millis for the cases where greater time
   fidelity is needed in a standardised way (javascript natively
   expects timestamps to be in milliseconds)
 - Included tests for the above
 - Deprecated the timestamp::option module as this achieves the same
   effect seamlessly
 - Maybe expanding this concept to other well-known serde types?
 - Replaces intermediate structs
 - Allows for generic serialising/deserialising from a single function
 - Currently implemented for:
    - Timestamp/TimestampMillis
    - T,Option<T>,Vec<T>,[T] (ser only)
@codecov
Copy link

codecov bot commented May 18, 2023

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%) ⬇️
time/src/serde/timestamp.rs 96.2% <95.4%> (-3.8%) ⬇️

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

@CaptainMaso
Copy link
Author

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
Copy link
Member

jhpratt commented Jun 2, 2023

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

  • PrimitiveDateTime should not be supported for timestamps, as they have no concept of time zones. It would probably be best to make the underlying type a generic, which would allow
  • Timestamps with milliseconds has been previously rejected as unnecessary. This is particularly so now that there is time::serde::format_description! and a [unix_timestamp] component. I'm open to reconsidering it as a serde format, but not as a method.
  • Is there any situation where a type would implement one trait but not the other?
  • Let's keep the new unit type and the traits inaccessible. They'll have to be public for the purposes of the API, but that doesn't mean a user needs the ability to access the items. This would avoid changing the public API in any manner.

@CaptainMaso
Copy link
Author

CaptainMaso commented Jun 5, 2023

  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
Copy link
Member

jhpratt commented Jun 10, 2023

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
Copy link
Member

jhpratt commented Jul 17, 2023

Would you mind providing an update?

@jhpratt jhpratt force-pushed the main branch 3 times, most recently from b963f68 to b1735de Compare July 20, 2023 09:25
@jhpratt jhpratt force-pushed the main branch 3 times, most recently from c8bc729 to 8060100 Compare July 30, 2023 09:57
@jhpratt
Copy link
Member

jhpratt commented Aug 2, 2023

Closing as inactive.

@jhpratt jhpratt closed this Aug 2, 2023
@jhpratt jhpratt added the C-stale label Aug 2, 2023
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.

well-known serde for Vec<OffsetDateTime>
2 participants