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

Serde rfc 2822 #1412

Closed
wants to merge 3 commits into from
Closed

Conversation

chrisranderson
Copy link

@chrisranderson chrisranderson commented Feb 4, 2024

Thanks for contributing to chrono!

If your feature is semver-compatible, please target the main branch;
for semver-incompatible changes, please target the 0.5.x branch.

I'm not sure what this means - every change is compatible with semver, right? Are you asking if it's a backwards-incompatible change?

Please consider adding a test to ensure your bug fix/feature will not break in the future.

Done.

@chrisranderson
Copy link
Author

Continuing from @pitdicker #1292 (comment):

I could comment on a few nits, but it looks pretty much like it should.
I believe all other deserialize implementations deserialize to DateTime? We should do so here to. DateTime discards the serialized offset info.
And all other (de)serializers support an Option variant.

I changed it to deserialize to DateTime<FixedOffset>. Not all other (de)serializers support an Option variant, though - the rfc3339 one doesn't, right?

Copy link

codecov bot commented Feb 4, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (cf17f7a) 91.72% compared to head (164cb44) 91.85%.
Report is 20 commits behind head on main.

Files Patch % Lines
src/datetime/serde.rs 93.02% 3 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@pitdicker pitdicker left a 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;
Copy link
Collaborator

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")
Copy link
Collaborator

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())
Copy link
Collaborator

@pitdicker pitdicker Feb 5, 2024

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.

@pitdicker
Copy link
Collaborator

Forgot to add: thank you for working on this!

And the changes are related enough to be squashed into one commit please.

@pitdicker
Copy link
Collaborator

@chrisranderson are you still interested in completing this PR?

@chrisranderson
Copy link
Author

No, sorry. Gratefully, I am now employed. Sorry to leave this hanging loose.

@pitdicker
Copy link
Collaborator

Congratulations! And no worries.

@pitdicker
Copy link
Collaborator

Opened #1477.

@pitdicker pitdicker closed this Mar 1, 2024
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