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

Migrate to new timezone database #81

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

dickermoshe
Copy link

@dickermoshe dickermoshe commented Feb 10, 2025

This PR introduces the following changes

  1. The convert function, which is basically a way to get an epoch from a local datetime (jan 1st is new york is at what timestamp) has had it arguments moved to a year, month day... which are more in line how to think about them and also remove the footgun of using a non-utc datetime when converting.
  2. Use a new timezone database provided by https://github.com/kshetline/tubular_time_tzdb which is more stable than it's dart counterpart. This fixes a number a bugs and web compatibility. It also allows us to calculate the dst for times past the end date of the timezone database using dst rules.
  3. Add an extensive Java test that rigorously check the results of timezone conversions.
  4. A small refactor to make sugar support more timezone providers in the future.
  5. span has been replaced by offset. This means users won't know the abbreviation of the current offset (DST or EST) or whether it is currently isDst. This is a change was made for the following reasons:
    a. Other timezone providers will have a hard time supporting this, there are very large disagreements what is considered isDst and in IMHO its best to avoid this issue completely.
    b. The span abbreviation would be really nice to have. We'll explore ways to bring it back. The issue is that DST rules don't have abbreviations associated with them.

  • Add java testing to CI

Copy link

codecov bot commented Feb 10, 2025

Codecov Report

Attention: Patch coverage is 94.44444% with 19 lines in your changes missing coverage. Please review.

Project coverage is 96.40%. Comparing base (b82ca85) to head (3424ac4).
Report is 83 commits behind head on master.

Files with missing lines Patch % Lines
...gar/lib/src/time/zone/providers/embedded/tzdb.dart 89.47% 8 Missing ⚠️
...ime/zone/providers/embedded/embedded_timezone.dart 96.21% 5 Missing ⚠️
sugar/lib/src/time/zone/timezone.dart 44.44% 5 Missing ⚠️
...gar/lib/src/time/zone/platform/posix_timezone.dart 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #81      +/-   ##
==========================================
+ Coverage   96.12%   96.40%   +0.28%     
==========================================
  Files          55       59       +4     
  Lines        1521     1890     +369     
==========================================
+ Hits         1462     1822     +360     
- Misses         59       68       +9     

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

Copy link
Member

@Pante Pante left a comment

Choose a reason for hiding this comment

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

I think generally we should try to process the raw data further at compile time. In addition, what is the tzupdate.jar used for?

@dickermoshe
Copy link
Author

I've added CI to test using Java.
I've also renamed the provider and refactored the db parsing from the actual timezone abject.

If you think this looks fine I'll add a generated Provider that loads everything at compile time

Copy link
Member

@Pante Pante left a comment

Choose a reason for hiding this comment

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

Generally LGTM! One quick question, but should we use microseconds or milliseconds as the most granular precision?

.github/workflows/sugar_unit_test.yaml Outdated Show resolved Hide resolved
.github/workflows/sugar_unit_test.yaml Show resolved Hide resolved
@dickermoshe
Copy link
Author

milliseconds is ok.

I did make one breaking change.
convert now takes a year,month day... looking object instead of a EpochMicroseconds.

@dickermoshe
Copy link
Author

Also, I will not be adding one that compiles the raw timezones because it bloats the binary massively for only a small improvement

@Pante
Copy link
Member

Pante commented Feb 11, 2025

I'll add an automated action that formats pull requests later today/early tomorrow.

@dickermoshe
Copy link
Author

In this latest release, timezone providers never return a "Factory" database and return null instead.
However the Timezone(...) constructor will return the "Factory" if it can't find it

// Breaking
Timezone.timezoneProvider["Factory"] // returns null

// Non-Breaking
final tz = Timezone("Factory"); // No such timezone, return factory
final tz = Timezone("Pizza");  // No such timezone, return factory

I can make this non-breaking if you want

@dickermoshe
Copy link
Author

I'll add an automated action that formats pull requests later today/early tomorrow.

There is tons of formatting that this repo need and doing so will turn this PR into a nightmare to review.
Make a independant formatting PR once this PR is merged

@Pante
Copy link
Member

Pante commented Feb 11, 2025

Yeah that’s a good point, this repo was created long before we standardised formatting etc internally, I’ll probably do it after this PR.

I think we should return factory instead of null!

@dickermoshe
Copy link
Author

I think we should return factory instead of null!

Sure

@dickermoshe
Copy link
Author

I think we should return factory instead of null!

Done.
I've also added a test that ensures that every timezone which worked in the previous version of sugar works in this new one.

@dickermoshe dickermoshe marked this pull request as ready for review February 11, 2025 04:38
@dickermoshe
Copy link
Author

CI should pass now. Ready for merging when (hopefully) passes

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.

Bad DST after 2038 Question about ZonedDateTime not supported in Chrome New Release & FFI
2 participants