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

Rename LocalResult to TzResolution, add alias #1501

Merged
merged 2 commits into from
Mar 12, 2024

Conversation

pitdicker
Copy link
Collaborator

The name LocalResult is unfortunate because in Rust types called *Result are used as return values for error cases.
However in our case it is not an error (at least not usually) but a choice of how to deal with time zone transitions due to for example DST.

We can do the rename on the 0.5.x branch or on main. If we think TzResolution is a better name that helps in understanding the use we may as well start using it on 0.4.x. With a type alias with the old name that is easy.

Doing the rename on main also has the advantage of keeping the branches a bit more similar. But if 0.5.x is a better target I'll rebase of course.

The first commit is a simple find-replace-rustfmt.
The second commit extends the documentation.

Copy link

codecov bot commented Mar 9, 2024

Codecov Report

Attention: Patch coverage is 38.88889% with 88 lines in your changes are missing coverage. Please review.

Project coverage is 91.80%. Comparing base (920e73b) to head (4189fb0).

Files Patch % Lines
src/offset/mod.rs 36.50% 40 Missing ⚠️
src/offset/local/tz_info/rule.rs 8.33% 22 Missing ⚠️
src/offset/local/tz_info/timezone.rs 16.66% 10 Missing ⚠️
src/format/parsed.rs 33.33% 4 Missing ⚠️
src/datetime/tests.rs 50.00% 3 Missing ⚠️
src/datetime/rustc_serialize.rs 50.00% 2 Missing ⚠️
src/offset/fixed.rs 50.00% 2 Missing ⚠️
src/offset/utc.rs 50.00% 2 Missing ⚠️
src/datetime/serde.rs 80.00% 1 Missing ⚠️
src/offset/local/mod.rs 50.00% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1501   +/-   ##
=======================================
  Coverage   91.80%   91.80%           
=======================================
  Files          40       40           
  Lines       18337    18338    +1     
=======================================
+ Hits        16835    16836    +1     
  Misses       1502     1502           

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

@pitdicker
Copy link
Collaborator Author

pitdicker commented Mar 9, 2024

@djc before spending effort on reviewing this, I'm interested in you opinion on #1448 (comment). Although the type becomes a bit more complex, it seems overall like a simplification to remove the offset methods from TimeZone and make the enum only generic over Offset types.

@djc
Copy link
Member

djc commented Mar 11, 2024

I don't love the name?

  • Abbreviations on type/field names are kinda unidiomatic
  • I don't think the term "resolution" is all that common for resolving a date/time to a timezone offset, at least in non-expert usage?

(I feel like it's probably slightly better than LocalResult but if we're going to do this I'd like to feel better about the name we end up picking.)

@pitdicker
Copy link
Collaborator Author

Hmm. The result of finding the offset of a local datetime within a timezone, which needs a decision on how to deal with times that fall in a transition. What more names can we think up for it?

ResolveOffset maybe? We need the user to decide what offset he wants in case of a transition.

@djc
Copy link
Member

djc commented Mar 11, 2024

CurrentOffset, ProjectedOffset, ExpectedOffset?

@pitdicker
Copy link
Collaborator Author

Those also don't seem to be it. The result is not current, a projection, and there may be no expectation? Resolve, resolution or result seem closer to me.

Maybe something that has 'map' or 'disambiguation' (https://peps.python.org/pep-0495/) in the name? I don't like something as long as disambiguation though.

@pitdicker
Copy link
Collaborator Author

LocalTimeMapping?

@djc
Copy link
Member

djc commented Mar 11, 2024

MappedTime?

@pitdicker
Copy link
Collaborator Author

pitdicker commented Mar 11, 2024

After looking at the dictionary Mapped is better than Mapping. Then I think we should keep Local in the name, just to make it more clear this is not a type like NaiveDateTime or DateTime. MappedLocalTime?

Seems a reasonable type to return from TimeZone::from_local_datetime. And it also seems explainable from DateTime::add_days or DateTime::with_hour and similar, which do the operation in local time and then have to calculate back an offset.

@djc
Copy link
Member

djc commented Mar 11, 2024

Seems like a decent option...

@pitdicker
Copy link
Collaborator Author

pitdicker commented Mar 11, 2024

The least bad 😆.

With this name I do think it is best to combine it with making the type only return DateTimes and not offsets anymore, the last idea in #1448 (comment). So targeting the 0.5 branch.

Have you already formed an opinion on that?
In short: We remove the TimeZone::offset_from_* methods. An offset can easily be derived from a DateTime with .offset(). That makes the TimeZone trait and this type simpler.

@djc
Copy link
Member

djc commented Mar 12, 2024

With this name I do think it is best to combine it with making the type only return DateTimes and not offsets anymore, the last idea in #1448 (comment). So targeting the 0.5 branch.

Why/how is that related to the naming? In principle I like the idea of bringing the new name to 0.4.x as an alias to potentially get some feedback.

Have you already formed an opinion on that? In short: We remove the TimeZone::offset_from_* methods. An offset can easily be derived from a DateTime with .offset(). That makes the TimeZone trait and this type simpler.

I have not, and this description doesn't help me form an opinion. Are there commits that flesh out this idea?

@pitdicker
Copy link
Collaborator Author

Why/how is that related to the naming?

It is a bit strange if TimeZone::offset_from_local_datetime returns an offset in a type with the name MappedLocalTime.
But I'm fine with doing it on 0.4, that method should be little-used.

I have not, and this description doesn't help me form an opinion. Are there commits that flesh out this idea?

I put effort in the description in the linked comment. But I'll try to make some commits after the rename.

@@ -13,16 +13,16 @@ impl<Tz: TimeZone> Encodable for DateTime<Tz> {
}
}

// lik? function to convert a LocalResult into a serde-ish Result
fn from<T, D>(me: LocalResult<T>, d: &mut D) -> Result<T, D::Error>
// lik? function to convert a MappedLocalTime into a serde-ish Result
Copy link
Member

Choose a reason for hiding this comment

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

"lik?"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have no clue. Remove it?

Copy link
Member

Choose a reason for hiding this comment

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

Yup.

/// which the local time is _missing_, or does not exist.
///
/// Chrono does not return a default choice or invalid data during time zone transitions, but has
/// the `MappedLocalTime` to help deal with the result correctly.
Copy link
Member

Choose a reason for hiding this comment

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

"has the MappedLocalTime to" -> "has the MappedLocalTime type to".

@pitdicker
Copy link
Collaborator Author

Something weird happened with my branches, I'll figure it out

@pitdicker pitdicker merged commit 1789183 into chronotope:main Mar 12, 2024
34 of 35 checks passed
@pitdicker pitdicker deleted the tz_resolution branch March 12, 2024 12:27
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