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

0.5: Convert API to return Result: Datelike::with_year #1466

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

Zomtir
Copy link
Contributor

@Zomtir Zomtir commented Feb 28, 2024

Follow up commit for #1444. Split from #1445.

  • Converts Datelike::with_year to return Result<T,E>
  • Converts the implementations NaiveDate::with_year and NaiveDateTime::with_year to return Result<T,E>
  • Converts the implementation DateTime<Tz>::with_year to return Result<T,E> as well
    • Adds DateTime<Tz>::map_local_result as transitionary helper function to prevent dependency on DateTime<Tz>::map_local
    • Adds LocalResult<T>::map_result_unique to have a transitionary, centralized mapping function until 0.5.x: Timezone conversion result (LocalResult) #1448 is settled

The function map_result_unique is NOT meant to outline future behaviour in any form. The idea is that there is some centralized mapping to a Result that can easily tracked for the purpose of #1448. Local (non-central) mappings will be prone to spread inconsistent behaviour.

However the future of LocalResult looks like, it will be easy to track the current usage and adapt the implementation and documentation by searching for the map_result_unique usage. For that reason I did not document the behaviour of map_result_unique anywhere else than in src/offset/mod.rs, but referenced it instead.

cc @pitdicker

Copy link

codecov bot commented Feb 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.03%. Comparing base (2e4e6ed) to head (c55ba4f).

Additional details and impacted files
@@            Coverage Diff             @@
##            0.5.x    #1466      +/-   ##
==========================================
- Coverage   94.04%   94.03%   -0.01%     
==========================================
  Files          37       37              
  Lines       17022    17005      -17     
==========================================
- Hits        16008    15991      -17     
  Misses       1014     1014              

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

@pitdicker
Copy link
Collaborator

@Zomtir Thank you!
Why do you keep trying to work on something that I consider one of the last pieces to convert, and don't pick something that is easy and ready without needing workarounds? I keep suggesting it friendly:

From #1444 (comment)

It seems good to do the conversion with only one method or a very small number of methods per commit. And starting with methods in the naive module that don't depend on pieces that have not yet been converted.

Converting LocalResult, ParseError and TimeDelta require some preparations first that I'll look into. Let's ignore anything around those for now.

From #1444 (comment)

Could you skip the DateTime type? The map_local type is going to pass on multiple error causes, such as OutOfRange, DoesNotExist, OS errors, and trouble with a TZ string or TZif file. I'd like to be a bit more careful with that type and leave that to its own PR.

We should also have a discussion around the Datelike and Timelike traits before converting them. For the naive types their signature is fine. For DateTime it would be better if they returned LocalResult to give users a way to deal with timezone issues (in my opinion). See #1050.

Repeated in #1445 (comment).

From #1445 (comment)

That is not addressing the problem. I cannot change the signature of an implementation without changing the signature of the trait.

True. That is why I suggested not touching the trait in this PR yet, and wait a bit.

And in #1445 (comment)

May I suggest the following methods as a first step ...

@Zomtir
Copy link
Contributor Author

Zomtir commented Feb 28, 2024

@pitdicker I have neither issue with the pull request idling until it gets useful and/or rebasing it once changes are made. Don't worry.

I am looking into from_num_days_from_ce as we speak and will leave a comment on #1445.

@pitdicker
Copy link
Collaborator

With #1490 these methods are no longer part of a trait and can be adjusted one type at a time.
Now is a good time to work on the methods on NaiveDate, NaiveTime and NaiveDateTime.
I'd still like to avoid DateTime.

@Zomtir
Copy link
Contributor Author

Zomtir commented Mar 9, 2024

I rebased the changes onto the the current 0.5.x. Now only NaiveDate and NaiveDateTime are affected.

  • API change of with_year to return Result
  • Linking is now more prominent in the documentation to follow the 'DRY' principle. There is little use in repeating the same information when the function has no own logic (e.g. NaiveDateTime::with_year directly calls NaiveDate::with_year).
  • For the same reason the examples in NaiveDateTime::with_year now only provides an example where the NaiveDateTime is used.

CC @pitdicker

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.

A couple of comments. I think it is mostly caused by the rebase?

src/naive/date/mod.rs Outdated Show resolved Hide resolved
src/naive/date/mod.rs Outdated Show resolved Hide resolved
src/naive/date/mod.rs Show resolved Hide resolved
src/naive/date/mod.rs Outdated Show resolved Hide resolved
src/naive/date/mod.rs Outdated Show resolved Hide resolved
src/naive/datetime/mod.rs Outdated Show resolved Hide resolved
src/naive/datetime/mod.rs Show resolved Hide resolved
@Zomtir Zomtir force-pushed the with_year branch 2 times, most recently from f204de1 to 0c97bfe Compare March 9, 2024 17:46
src/naive/date/mod.rs Outdated Show resolved Hide resolved
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.

Very nice!

src/naive/date/mod.rs Outdated Show resolved Hide resolved
@djc djc merged commit ceeb289 into chronotope:0.5.x Mar 12, 2024
35 checks passed
@Zomtir Zomtir deleted the with_year branch March 12, 2024 19:28
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.

3 participants