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

Revert #806 #1318

Merged
merged 2 commits into from
Sep 26, 2023
Merged

Revert #806 #1318

merged 2 commits into from
Sep 26, 2023

Conversation

pitdicker
Copy link
Collaborator

@pitdicker pitdicker commented Sep 26, 2023

#806 conveniently adds parsing methods to DateTime<Utc>. I don't believe it was fully realized at the time how much of a change this is going to be for users.

All parse_from_* methods are going to need type annotations on 0.5. These are widely used methods. I think this makes the common use more annoying.

Parsing to DateTime<FixedOffset> is what I consider the regular case. It is the type that best matches the format strings of RFC 3339, RFC 2822, ISO 8601, and any manual string that can be parsed as a DateTime.

// 0.4.x
let dt = DateTime::parse_from_rfc3339("2015-06-30T23:56:05+02:00")?;
let dt: DateTime<Utc> = DateTime::parse_from_rfc3339("2015-06-30T23:56:05+02:00")?.into();

// with #806:
let dt = DateTime::<FixedOffset>::parse_from_rfc3339("2015-06-30T23:56:05+02:00")?;
let dt = DateTime::<Utc>::parse_from_rfc3339("2015-06-30T23:56:05+02:00")?;

Parsing to DateTime<Utc> folds the offset in the datetime value, essentially losing the offset information.
In #1075 the discussion was to go in the opposite direction for 0.5: if we parse a value with fields that the target type can't hold, we should return an error.

So I would like to revert for two reasons: all the extra type annotations, and to make it an explicit choice for users to drop the offset information.

cc @mqudsi.

@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

Merging #1318 (d9c3772) into main (d23e049) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1318   +/-   ##
=======================================
  Coverage   91.48%   91.48%           
=======================================
  Files          35       35           
  Lines       16860    16826   -34     
=======================================
- Hits        15425    15394   -31     
+ Misses       1435     1432    -3     
Files Coverage Δ
src/datetime/mod.rs 85.22% <100.00%> (+0.12%) ⬆️
src/datetime/tests.rs 96.41% <100.00%> (-0.05%) ⬇️
src/format/parse.rs 96.83% <100.00%> (ø)
src/format/strftime.rs 98.94% <100.00%> (-0.01%) ⬇️
src/lib.rs 95.58% <100.00%> (ø)
src/naive/time/mod.rs 94.63% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pitdicker pitdicker mentioned this pull request Sep 26, 2023
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

What is our most convenient way to convert from DateTime<FixedOffset> to DateTime<Utc>? Perhaps if we make that easy enough it compensates for this?

Should we deprecate these on 0.4.x?

@pitdicker
Copy link
Collaborator Author

What is our most convenient way to convert from DateTime<FixedOffset> to DateTime<Utc>?

I think let dt: DateTime<Utc> = DateTime::parse_from_rfc3339("2015-06-30T23:56:05+02:00")?.into(); is the best we have now, into() doesn't take type annotations.

Perhaps if we make that easy enough it compensates for this?

We can add a method similar to DateTime::fixed_offset(). I am not sure about the best name. Maybe DateTime::utc_only(), to show it forgets about the offset?

Should we deprecate these on 0.4.x?

There is no equivalent on 0.4.x.

@djc
Copy link
Member

djc commented Sep 26, 2023

But it doesn't forget, right? It just folds it into the time? I was thinking it might be called into_utc().

@pitdicker
Copy link
Collaborator Author

I don't know, but kind of want to make it clear this is a non-reversible conversion. But well, so is DateTime::fixed_offset() if you are converting from a chrono-tz timezone.

From the API guidelines:

Conversions prefixed as_ and into_ typically decrease abstraction, either exposing a view into the underlying representation (as) or deconstructing data into its underlying representation (into). Conversions prefixed to_, on the other hand, typically stay at the same level of abstraction but do some work to change from one representation to another.

Sound like we should go with to_*. But into_utc() matches nicely with the current into() 👍.

@pitdicker pitdicker merged commit ce4644f into chronotope:main Sep 26, 2023
37 checks passed
@pitdicker pitdicker deleted the revert_806 branch September 26, 2023 14:31
@djc
Copy link
Member

djc commented Sep 26, 2023

Ah yeah, I suppose to_utc() makes more sense.

@pitdicker pitdicker mentioned this pull request Sep 26, 2023
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