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 DateTime::try_to_rfc2822, deprecate DateTime::to_rfc2822 #1330

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

Conversation

pitdicker
Copy link
Collaborator

Split out from #1144.

This adds a new method try_to_rfc2822 that returns None instead of panicking on years outside of 0..9999.

@codecov
Copy link

codecov bot commented Sep 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.82%. Comparing base (3114597) to head (da1ff7e).

❗ Current head da1ff7e differs from pull request most recent head e980391. Consider uploading reports for the commit e980391 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1330      +/-   ##
==========================================
+ Coverage   91.80%   91.82%   +0.01%     
==========================================
  Files          40       40              
  Lines       18329    18352      +23     
==========================================
+ Hits        16827    16851      +24     
+ Misses       1502     1501       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pitdicker pitdicker force-pushed the try_to_rfc2822 branch 3 times, most recently from 14037a1 to da1ff7e Compare March 6, 2024 19:26
@pitdicker
Copy link
Collaborator Author

@djc Sorry for all the pushes.
This PR and #1331 expose two more methods that can fail, that would be nice to convert to return Results on 0.5.

I don't depend on them for any other work. But maybe nice to keep in mind for reviewing? They should not do anything surprising, it is more a matter of: do we want to deprecate them on 0.4?

@djc
Copy link
Member

djc commented Mar 7, 2024

So my main hesitation about this and #1331 has been that I find the try_to_rfc2822() method name ugly/unidiomatic. I wonder if there's something else we could do here that would clarify how RFC 2822-compatible datetimes are really a (strict?) subset of the values supported by our DateTime type, and if that could also help reducing fallibility in a new format API down the line.

For example, maybe we have a Rfc2822 wrapper for DateTime and this becomes a TryFrom impl?

@pitdicker
Copy link
Collaborator Author

For example, maybe we have a Rfc2822 wrapper for DateTime and this becomes a TryFrom impl?

I have to think about this for a bit.

@pitdicker
Copy link
Collaborator Author

So my main hesitation about this and #1331 has been that I find the try_to_rfc2822() method name ugly/unidiomatic. I wonder if there's something else we could do here that would clarify how RFC 2822-compatible datetimes are really a (strict?) subset of the values supported by our DateTime type, and if that could also help reducing fallibility in a new format API down the line.

For example, maybe we have a Rfc2822 wrapper for DateTime and this becomes a TryFrom impl?

I'm not too fond of the name either. Maybe we should just make the to_rfc2822() method infallible on 0.5 and leave it as it is for 0.4.x? 0.5 feels less far away than half a year ago.

A wrapper type such as Rfc2822 to encode the invariant that a DateTime is in the range 0..10_000 does not seem all that useful to me. There is no other use for it except formatting. Then I'd rather have a fallible formatting method than a fallible conversion to another type as an extra step.

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