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 MappedLocalTime::try_map #1533

Merged
merged 1 commit into from
Mar 22, 2024
Merged

Add MappedLocalTime::try_map #1533

merged 1 commit into from
Mar 22, 2024

Conversation

pitdicker
Copy link
Collaborator

@pitdicker pitdicker commented Mar 21, 2024

Passing on one error in unix::Cache::offset requires a match that is very similar to the one in TimeZone::from_local_datetime.
I added MappedLocalTime::try_map that returns MappedLocalTime::None that can be used in both places.

For the 0.5.x branch it will be easy to convert this method to return a Result instead.

Copy link

codecov bot commented Mar 21, 2024

Codecov Report

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

Project coverage is 91.82%. Comparing base (3adfd88) to head (e8db5cd).

Files Patch % Lines
src/offset/mod.rs 73.33% 4 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1533   +/-   ##
=======================================
  Coverage   91.82%   91.82%           
=======================================
  Files          40       40           
  Lines       18345    18345           
=======================================
  Hits        16846    16846           
  Misses       1499     1499           

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

Copy link
Contributor

@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.

This PR on its own doesn't look well-motivated... I'd probably keep this commit in the Cache PR but am okay if you want to get it merged first.

///
/// Returns `MappedLocalTime::None` if the function returns `None`.
#[must_use]
pub(crate) fn try_map<U, F: FnMut(T) -> Option<U>>(self, mut f: F) -> MappedLocalTime<U> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the Option variant of this is called and_then(), maybe reuse that name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍 and_then() is better.

@pitdicker
Copy link
Collaborator Author

This PR on its own doesn't look well-motivated...

Not sure what you mean. Maybe as another motivation: to implement addition and substraction for a CalendarDuration (#1282) months and days should be added in localtime with a MappedLocalTime as result. Then seconds should be added using a method such as this so the type remains a MappedLocalTime.

But I have to admit I'm not sure if the current two uses in this PR for the 0.4 versions will remain useful for 0.5 with one of the plans in 1448#issuecomment-1987220921. So the method is pub(crate) for now.

@djc
Copy link
Contributor

djc commented Mar 22, 2024

Ah, sorry, I missed that there are sort of two users for the new method. Looks a little surprising because one of the callers did not previously have the extensive logic implemented in the method.

@pitdicker
Copy link
Collaborator Author

Yes, that was the issue I set out to fix 😄. Sorry for not saying so better.

@pitdicker pitdicker merged commit e05ba8b into chronotope:main Mar 22, 2024
34 of 35 checks passed
@pitdicker pitdicker deleted the try_map branch March 22, 2024 11:19
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.

None yet

2 participants