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 Timestamp decode/encode #351

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

junderw
Copy link

@junderw junderw commented Aug 18, 2024

This is a baseline just to have something to work off of, very WIP.

  1. Should we perhaps just use DateTime from the chrono crate under a feature flag instead of creating a simple enum. (I've decided against this, it's too much of a hassle.)
  2. How should we go about including it in rmp-serde if at all? (Made an initial implementation, but kind of hacky, awaiting feedback)
  3. Checking for invalid values (ie. nanos above 999_999_999) should probably be done better. I am not too familiar with this library and how I should be propagating such errors. (Especially with serde.)
  4. Also, please advise on any formatting requests. (I turned off rustfmt)

Let me know if this isn't something you're interested it.

@junderw junderw marked this pull request as draft August 18, 2024 15:00
@junderw junderw force-pushed the junderw/add-timestamps branch from 1d51bf6 to bac67df Compare August 20, 2024 17:55
@junderw junderw changed the title WIP: Adding Timestamp decode/encode Adding Timestamp decode/encode Aug 26, 2024
@junderw junderw marked this pull request as ready for review August 26, 2024 15:48
@jcuffe
Copy link

jcuffe commented Nov 24, 2024

@junderw thank you for getting this started 🙏 I'm interested in using this with serde, but I don't understand either library well enough to know what code to add to rmp_serde so that I can use rmp::Timestamp in my structs. Would you be interested in collaborating on this?

@junderw
Copy link
Author

junderw commented Nov 27, 2024

@jcuffe I added a hackey TimestampSerde wrapper. It's a simple wrapper only for serde serialize and deserialize.

The content is pub, so you can just take the content out after it's deserialized.

I added a test for each way, you can look at the test to see how it works.

@junderw junderw force-pushed the junderw/add-timestamps branch from f4bacae to d66e6ca Compare November 27, 2024 14:57
@junderw junderw force-pushed the junderw/add-timestamps branch from d66e6ca to 48d2eb1 Compare November 27, 2024 15:46
@jcuffe
Copy link

jcuffe commented Nov 27, 2024

Excellent! Thank you @junderw 😁

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.

2 participants