-
Notifications
You must be signed in to change notification settings - Fork 533
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
Serde rfc 2822 #1412
Serde rfc 2822 #1412
Conversation
Continuing from @pitdicker #1292 (comment):
I changed it to deserialize to |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1412 +/- ##
==========================================
+ Coverage 91.72% 91.85% +0.12%
==========================================
Files 38 38
Lines 17266 17561 +295
==========================================
+ Hits 15838 16130 +292
- Misses 1428 1431 +3 ☔ View full report in Codecov by Sentry. |
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.
We are inevitably going to get a request to add an optional variant. But lets get this ready first.
@@ -24,6 +24,10 @@ pub struct MicroSecondsTimestampVisitor; | |||
#[derive(Debug)] | |||
pub struct MilliSecondsTimestampVisitor; | |||
|
|||
#[doc(hidden)] | |||
#[derive(Debug)] | |||
pub struct Rfc2822Visitor; |
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.
I believe you can make this private. The others are public for backwards compatibility.
type Value = DateTime<FixedOffset>; | ||
|
||
fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result { | ||
formatter.write_str("an RFC 2822 (email) formatted datetime string") |
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.
Personally I would remove (email)
.
where | ||
S: ser::Serializer, | ||
{ | ||
serializer.serialize_str(&dt.to_rfc2822()) |
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.
In format/formatting.rs
there is a write_rfc2822
function that works without allocating. Using that will make this serializer work on no_std
targets like the others (and make the CI pass).
You will have to adjust its #[cfg(feature = "alloc")]
to also enable it with the serde
feature.
Forgot to add: thank you for working on this! And the changes are related enough to be squashed into one commit please. |
@chrisranderson are you still interested in completing this PR? |
No, sorry. Gratefully, I am now employed. Sorry to leave this hanging loose. |
Congratulations! And no worries. |
Opened #1477. |
I'm not sure what this means - every change is compatible with semver, right? Are you asking if it's a backwards-incompatible change?
Done.