-
Notifications
You must be signed in to change notification settings - Fork 2.8k
fix(index): Apply feedback from Cargo team #16369
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
Changes from all commits
a0dc7eb
fd294ac
7bccc12
9a3f316
b8e73e6
e31f446
73ae7d5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,7 +49,13 @@ pub struct IndexPackage<'a> { | |
| /// The publish time for the package. Unstable. | ||
| /// | ||
| /// In ISO8601 with UTC timezone (e.g. 2025-11-12T19:30:12Z) | ||
| pub pubtime: Option<String>, | ||
| /// | ||
| /// This should be the original publish time and not changed on any status changes, | ||
| /// like [`IndexPackage::yanked`]. | ||
| #[cfg_attr(feature = "unstable-schema", schemars(with = "Option<String>"))] | ||
| #[serde(with = "serde_pubtime")] | ||
| #[serde(default)] | ||
| pub pubtime: Option<jiff::Timestamp>, | ||
| /// The schema version for this entry. | ||
| /// | ||
| /// If this is None, it defaults to version `1`. Entries with unknown | ||
|
|
@@ -117,6 +123,76 @@ pub struct RegistryDependency<'a> { | |
| pub lib: bool, | ||
| } | ||
|
|
||
| pub fn parse_pubtime(s: &str) -> Result<jiff::Timestamp, jiff::Error> { | ||
| let dt = jiff::civil::DateTime::strptime("%Y-%m-%dT%H:%M:%SZ", s)?; | ||
| if s.len() == 20 { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cargo isn't future-proof for years beyond 9999 😆
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I hope it can last that long! Perhaps this code still is buried in the Arctic mountain at that time🤣 |
||
| let zoned = dt.to_zoned(jiff::tz::TimeZone::UTC)?; | ||
0xPoe marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| let timestamp = zoned.timestamp(); | ||
| Ok(timestamp) | ||
| } else { | ||
| Err(jiff::Error::from_args(format_args!( | ||
| "padding required for `{s}`" | ||
| ))) | ||
| } | ||
| } | ||
|
|
||
| pub fn format_pubtime(t: jiff::Timestamp) -> String { | ||
| t.strftime("%Y-%m-%dT%H:%M:%SZ").to_string() | ||
| } | ||
|
|
||
| mod serde_pubtime { | ||
| #[inline] | ||
| pub(super) fn serialize<S: serde::Serializer>( | ||
| timestamp: &Option<jiff::Timestamp>, | ||
| se: S, | ||
| ) -> Result<S::Ok, S::Error> { | ||
| match *timestamp { | ||
| None => se.serialize_none(), | ||
| Some(ref ts) => { | ||
| let s = super::format_pubtime(*ts); | ||
| se.serialize_str(&s) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[inline] | ||
| pub(super) fn deserialize<'de, D: serde::Deserializer<'de>>( | ||
| de: D, | ||
| ) -> Result<Option<jiff::Timestamp>, D::Error> { | ||
| de.deserialize_option(OptionalVisitor( | ||
| serde_untagged::UntaggedEnumVisitor::new() | ||
| .expecting("date time") | ||
| .string(|value| super::parse_pubtime(&value).map_err(serde::de::Error::custom)), | ||
| )) | ||
| } | ||
|
|
||
| /// A generic visitor for `Option<DateTime>`. | ||
| struct OptionalVisitor<V>(V); | ||
|
|
||
| impl<'de, V: serde::de::Visitor<'de, Value = jiff::Timestamp>> serde::de::Visitor<'de> | ||
| for OptionalVisitor<V> | ||
| { | ||
| type Value = Option<jiff::Timestamp>; | ||
|
|
||
| fn expecting(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { | ||
| f.write_str("date time") | ||
| } | ||
|
|
||
| #[inline] | ||
| fn visit_some<D: serde::de::Deserializer<'de>>( | ||
| self, | ||
| de: D, | ||
| ) -> Result<Option<jiff::Timestamp>, D::Error> { | ||
| de.deserialize_str(self.0).map(Some) | ||
| } | ||
|
|
||
| #[inline] | ||
| fn visit_none<E: serde::de::Error>(self) -> Result<Option<jiff::Timestamp>, E> { | ||
| Ok(None) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn default_true() -> bool { | ||
| true | ||
| } | ||
|
|
@@ -161,3 +237,41 @@ fn dump_index_schema() { | |
| let dump = serde_json::to_string_pretty(&schema).unwrap(); | ||
| snapbox::assert_data_eq!(dump, snapbox::file!("../index.schema.json").raw()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn pubtime_format() { | ||
| use snapbox::str; | ||
|
|
||
| let input = [ | ||
| ("2025-11-12T19:30:12Z", Some(str!["2025-11-12T19:30:12Z"])), | ||
| // Padded values | ||
| ("2025-01-02T09:03:02Z", Some(str!["2025-01-02T09:03:02Z"])), | ||
| // Alt timezone format | ||
| ("2025-11-12T19:30:12-04", None), | ||
| // Alt date/time separator | ||
| ("2025-11-12 19:30:12Z", None), | ||
| // Non-padded values | ||
| ("2025-11-12T19:30:12+4", None), | ||
| ("2025-1-12T19:30:12+4", None), | ||
| ("2025-11-2T19:30:12+4", None), | ||
| ("2025-11-12T9:30:12Z", None), | ||
| ("2025-11-12T19:3:12Z", None), | ||
| ("2025-11-12T19:30:2Z", None), | ||
| ]; | ||
| for (input, expected) in input { | ||
| let (parsed, expected) = match (parse_pubtime(input), expected) { | ||
| (Ok(_), None) => { | ||
| panic!("`{input}` did not error"); | ||
| } | ||
| (Ok(parsed), Some(expected)) => (parsed, expected), | ||
| (Err(err), Some(_)) => { | ||
| panic!("`{input}` did not parse successfully: {err}"); | ||
| } | ||
| _ => { | ||
| continue; | ||
| } | ||
| }; | ||
| let output = format_pubtime(parsed); | ||
| snapbox::assert_data_eq!(output, expected); | ||
| } | ||
| } | ||
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.
Do we intend to expose the concrete
jiff::Timestampto users? Should we wrap it with our own type?(Similar discussion in #15293 (comment). I am fine with either)
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.
My comment from that thread
Just adding a field is a breaking change so not to worried and adding a custom type seems like boiler plate with little benefit
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.
Thanks. Just want to make sure this is intentional. Also
cargo-util-schemasis a package we are not worried about bumping version.