search

chronotope / chrono

Date and time library for Rust
Other
3.3k stars 523 forks source link

Refactor `unix::Cache` #1529

Open pitdicker opened 6 months ago

pitdicker commented 6 months ago

I put in the effort to slice the first commit of #1457 into 17 commits that each are readable as a refactor.

The goal is to move all logic for determining and parsing the time zone with fallbacks in one layer. This will make it possible to properly hook up errors, and to make the cache invalidation work beyond the most basic cases. But those improvements are not included in this PR.

The last commit, adding support for the TZ_DIR variable, is the only noticeable change in functionality.

djc commented 6 months ago

Thanks for doing the legwork on this! Will try to take a look soon.

codecov[bot] commented 6 months ago

Codecov Report

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

Project coverage is 91.52%. Comparing base (ffe2745) to head (9ba3069). Report is 7 commits behind head on main.

Files Patch % Lines
src/offset/local/unix.rs 48.43% 66 Missing :warning:
src/offset/local/tz_info/timezone.rs 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1529 +/- ## ========================================== - Coverage 91.79% 91.52% -0.27% ========================================== Files 37 37 Lines 18175 18153 -22 ========================================== - Hits 16683 16614 -69 - Misses 1492 1539 +47 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

pitdicker commented 6 months ago

Of course, and thank you.

Removed one commit that in the end didn't do much.

As a note: the reason to make everything methods on the Cache type is because as a follow-up I not only want to set self.zone but also for every source that is tried the inputs we should watch to invalidate the cache.

pitdicker commented 6 months ago

I've tried to review this, but I've found it challenging.

Most of the changes don't look like improvements to me -- they mostly add more code, and seem to make it all more complex. Tell me why/how I'm wrong?

When looking at it commit by commit it looks that way. Once you get to "Move parsing of TZ variable to unix module" we can start to share code that the previous commits where setting up for.

And this mostly prepares for the final commit in #1457. Could you give the end result a global look?

djc commented 6 months ago

I'm really sorry but even after looking at all of the changes in #1457 today it doesn't seem more clear -- and it adds 80 lines of code, is that all for the new functionality? That seems to remove some tests so, if anything, seems to be undercounting library code. It seems to add lots of small functions which IMO just make it harder to follow the flow.