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

Simplify lazy initialisation of TimezoneService. #654

Merged
merged 1 commit into from
Apr 9, 2024

Conversation

darktrojan
Copy link
Collaborator

It appears the current way of bootstrapping the core zones exists because TimezoneService's position among the modules means we can't call reset immediately.

Something seem to not work right and leads to Time.epochTime being in the floating time zone instead of UTC. At least when run in Thunderbird, I'm not sure if it always happens. My patch just creates the zones on first use.

@darktrojan darktrojan force-pushed the timezone-service-startup branch from d440f1a to ce38022 Compare April 3, 2024 23:57
It appears the current way of bootstrapping the core zones exists because TimezoneService's position among the modules means we can't call reset immediately.

Something seem to not work right and leads to Time.epochTime being in the floating time zone instead of UTC. At least when run in Thunderbird, I'm not sure if it always happens.
My patch just creates the zones on first use.
@darktrojan darktrojan force-pushed the timezone-service-startup branch from ce38022 to ced32d8 Compare April 4, 2024 00:04
@darktrojan
Copy link
Collaborator Author

Oops, I failed at git.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 8547113242

Details

  • 13 of 21 (61.9%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.07%) to 98.024%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/ical/timezone_service.js 13 21 61.9%
Totals Coverage Status
Change from base Build 8513693962: -0.07%
Covered Lines: 9287
Relevant Lines: 9458

💛 - Coveralls

Copy link
Collaborator

@leftmostcat leftmostcat left a comment

Choose a reason for hiding this comment

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

This looks sound to me and I think getting rid of side effects for getting one of the three special zones is a big improvement.

@darktrojan darktrojan merged commit b7e42f7 into kewisch:main Apr 9, 2024
7 checks passed
@darktrojan darktrojan deleted the timezone-service-startup branch April 18, 2024 00:58
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