Refactor unix::Cache

[pitdicker]

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]

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

[codecov]

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 ⚠️
src/offset/local/tz_info/timezone.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

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

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

[pitdicker]

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.

[djc]

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?

[djc]

Why is this better? It seems less precise to me.

[pitdicker]

Hmm, maybe I should keep an mtime around. It is not guaranteed to be the real modification time. We should either use ctime or keep track of the mtime.

Comparing ctime to last_checked is simpler though.

[djc]

The duplication between here and needs_update() doesn't seem great...

[pitdicker]

Thought the same! But it seemed premature optimization to remove this one duplicate read of an environment variable. We only do so if the variable has changed, which should be rare.

We can keep the variable around of course. It will have to be returned or passed to a couple of methods. More than what is visible in this PR as the final commit in #1457 adds more methods.

[djc]

Can we just represent this as Option<Source> instead? Seems more idiomatic to me.

[pitdicker]

I'll update it to use an Option.

[djc]

Why? This doesn't seem like an improvement.

[pitdicker]

It is a preparation for the next commit: splitting up the offset function.
The old code did all in the initialization in offset() using Cache::default on first use within a thread.

The offset_from_utc_datetime and offset_from_local_datetime both had to go through the offset function even though they had little in common (and preferably not the same return type), so they could use the same get_or_insert_with.

The goal of this commit is to move the concern of initialization from the caller the Cache type.

[pitdicker]

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]

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.