Skip to content

Conversation

abr-egn
Copy link
Contributor

@abr-egn abr-egn commented Aug 19, 2025

RUST-2251

This adds toplevel documentation to the serde_helpers module; I also removed some now-redundant (and, I think, arguably incorrect) serde_with trait impls from elsewhere and made it so that the converters can be used without serde_with.

@@ -496,31 +491,6 @@ impl<T: chrono::TimeZone> From<chrono::DateTime<T>> for crate::DateTime {
}
}

#[cfg(all(feature = "chrono-0_4", feature = "serde_with-3"))]
impl<'de> serde_with::DeserializeAs<'de, chrono::DateTime<Utc>> for crate::DateTime {
Copy link
Contributor Author

@abr-egn abr-egn Aug 19, 2025

Choose a reason for hiding this comment

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

DeserializeAs/SerializeAs are supposed to only be implemented for container types or converter types, not leaf value types; this was essentially tagging crate::DateTime as a converter type for itself.

@abr-egn abr-egn marked this pull request as ready for review August 19, 2025 13:02
@abr-egn abr-egn requested a review from a team as a code owner August 19, 2025 13:02
@abr-egn abr-egn requested a review from isabelatkinson August 19, 2025 13:02
Copy link
Contributor

@isabelatkinson isabelatkinson left a comment

Choose a reason for hiding this comment

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

the updated docs look great! I have a few additional random suggestions from poking around in the generated docs: isabelatkinson@3c9ebc0

@abr-egn abr-egn force-pushed the RUST-2251/serde-helpers-doc branch from 3c9ebc0 to f721029 Compare August 20, 2025 09:52
@abr-egn abr-egn force-pushed the RUST-2251/serde-helpers-doc branch from f721029 to 7b3ebec Compare August 20, 2025 09:58
@abr-egn abr-egn requested a review from isabelatkinson August 20, 2025 10:08
@abr-egn
Copy link
Contributor Author

abr-egn commented Aug 20, 2025

the updated docs look great! I have a few additional random suggestions from poking around in the generated docs: isabelatkinson@3c9ebc0

Applied, thanks :). I can't seem to both fix the merge conflict and preserve the verified status of your patch but I think since it's verified with my key it's fine?

Copy link
Contributor

@isabelatkinson isabelatkinson left a comment

Choose a reason for hiding this comment

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

but I think since it's verified with my key it's fine?

yeah I think that should be fine!

@abr-egn abr-egn merged commit 5c20bde into mongodb:main Aug 20, 2025
12 checks passed
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