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 support for serialising and deserialising string timestamps #1067

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

sinking-point
Copy link

@sinking-point sinking-point commented May 16, 2023

This PR aims to address #196 (comment)

  • Implement ts_seconds_string (in DateTime)
  • Implement ts_milliseconds_string
  • Implement ts_microseconds_string
  • Implement ts_nanoseconds_string
  • Add optionals (e.g. ts_seconds_string_option)
  • Support NaiveDateTime as well

@sinking-point
Copy link
Author

@djc Is my work so far on the right track? Would you like to request any changes to my approach?

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach looks okay to me, please clean up the commit history a bit by squashing (I usually use an interactive rebase).

src/datetime/serde.rs Show resolved Hide resolved
src/datetime/serde.rs Show resolved Hide resolved
src/datetime/serde.rs Show resolved Hide resolved
@sinking-point
Copy link
Author

Thanks for the feedback @djc . There might be some more messy commits as I continue working on this, but I'll ensure to clean them up when I'm done.

@sinking-point sinking-point force-pushed the sinking-point/serde-string-timestamp-196 branch from b1eb8c2 to 172ea3d Compare May 17, 2023 08:21
@djc djc marked this pull request as draft May 17, 2023 12:00
@djc djc changed the title (WIP) Add support for serialising and deserialising string timestamps Add support for serialising and deserialising string timestamps May 17, 2023
@djc
Copy link
Member

djc commented May 17, 2023

I've changed this to draft status for now, please switch it to ready once you think it's ready (or just comment if you want feedback on something sooner).

@sinking-point sinking-point force-pushed the sinking-point/serde-string-timestamp-196 branch 3 times, most recently from 225265c to 0a8676b Compare May 18, 2023 08:15
@sinking-point sinking-point force-pushed the sinking-point/serde-string-timestamp-196 branch from 9e56402 to d454d81 Compare May 18, 2023 17:15
@sinking-point sinking-point marked this pull request as ready for review May 18, 2023 17:16
@sinking-point sinking-point requested a review from djc May 18, 2023 17:17
@pitdicker
Copy link
Collaborator

@sinking-point I have only glanced at the code, apologies for that.
The amount of lines needed for (de)serializing is massive! Is there a way to collapse that into something manageable with a macro?

@sinking-point
Copy link
Author

@pitdicker A lot of those lines are documentation and tests. I could potentially try to define a macro, but the tests and docs would remain.

@sinking-point
Copy link
Author

The reason there's so much repetition is that there are 4 dimensions: int/string, normal/optional, DateTime/NaiveDateTime, nano/micro/milli/seconds. I agree that this isn't very DRY and it would be good to find a DRYer way of doing it.

@sinking-point
Copy link
Author

TBH this is difficult without extending serde's field attributes. It would be nice if there was a via attribute that takes another type (e.g. TimestampNanoseconds). Serde would deserialise into that first, then into() it to the target type (e.g. DateTime).

@sinking-point
Copy link
Author

There is already a similar feature in the container attributes: from and into.

@sinking-point
Copy link
Author

Here's the usage I'm envisioning:

use chrono::{TimeZone, DateTime, Utc, NaiveDate};
use serde_derive::{Deserialize, Serialize};
use chrono::serde::{Timestamp, Nanoseconds};
#[derive(Deserialize, Serialize)]
struct S {
    #[serde(via = "Timestamp<Nanoseconds, i64>")] // could equally be Timestamp<Seconds, String>
    time: DateTime<Utc> // could equally be NaiveDateTime - both would implement From<Timestamp>
}

Maybe Timestamp could even take a timezone parameter as well, which would allow automatic conversion to the timezone of the DateTime.

To reiterate, this via attribute doesn't exist yet. We'd need to get it into serde for this idea to be possible.

@djc
Copy link
Member

djc commented May 19, 2023

Given that the with attribute just points to deserialize() and serialize() functions in a module, there's a a lot of freedom with what we can do within those functions. So it feels like we should be able to do your Timestamp<Nanoseconds, i64> thing with some minimal boilerplate wrapping?

(Or macros, which would probably make this easier if also a little uglier. BTW, I don't see why we couldn't have the macros generate the tests, too.)

@sinking-point
Copy link
Author

We could, but if we still want to have docs and tests for each variant then it wouldn't reduce the line count by all that much.

In my opinion, tests should be concrete. I feel that tests are best when they're a self-contained, readable specification of what a module should do. Also, one function of the tests would be to test the assumptions made by the programmer in defining the macros. However, if the tests are also defined by a macro then they will be influenced by those same assumptions, which might cancel out.

Have you seen my comment on the issue? It seems that implementing my proposal would almost just be reimplementing part of the serde_with crate.

@djc
Copy link
Member

djc commented May 19, 2023

I did see it. I'm fine with closing this PR or maybe substituting it with a documentation pointer to the serde_with crate.

Copy link
Contributor

@jtmoon79 jtmoon79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have tests for maximum and minimum values, and more importantly, values just outside those boundaries.


Ok(())
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about test cases

  • using MAX_UTC, MIN_UTC, which should return a values for each
  • and the two values just one outside those boundaries, which should return None

It'd be good to include those tests. e.g. fn test_deserialize_ts_nanoseconds_string_max_min() ... and fn test_serialize_ts_nanoseconds_string_max_min() ...


Ok(())
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question about maximum and minimum values, and the value immediately outside those boundaries.


Ok(())
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question about maximum and minimum values, and the value immediately outside those boundaries.


Ok(())
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question about maximum and minimum values, and the value immediately outside those boundaries.


Ok(())
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question about maximum and minimum values, and the value immediately outside those boundaries.


Ok(())
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question about maximum and minimum values, and the value immediately outside those boundaries.


Ok(())
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question about testing maximum and minimum boundaries, and values immediately outside those boundaries.


assert_eq!(my_s.time.timestamp(), 1431684000);

Ok(())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question about testing maximum and minimum boundaries, and values immediately outside those boundaries.


Ok(())
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question about testing maximum and minimum boundaries, and values immediately outside those boundaries.


Ok(())
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question about testing maximum and minimum boundaries, and values immediately outside those boundaries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants