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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support both tzinfo v1 and v2 alongwith non-half hour offsets. #8880

Merged
merged 3 commits into from Dec 8, 2021

Conversation

philr
Copy link
Contributor

@philr philr commented Nov 14, 2021

This is a 馃檵 feature or enhancement.

Summary

This pull request changes WinTZ.calculate to support use with tzinfo v2 as well as the currently supported v1. Instead of determining the offset from the difference between local time and UTC, it now uses the defined offset returned by tzinfo (in a v1 and v2 compatible way).

The offset minutes calculation has also been changed to allow for time zones that don't use hour or half hour offsets. For example, Australia/Eucla's is at +08:45. Before the change, this would have resulted in WTZ-08:30 instead of the correct WTZ-08:45.

The new gemfile_contents and Windows documentation have updated to allow use with either tzinfo v1 or v2.

Unit tests for WinTZ have been added. This required adding tzinfo and tzinfo-data to the :test group in the Gemfile.

Context

Resolves #8516.

Use the defined offset (in a v1 and v2 compatible way) instead of
determining the offset from the difference between local time and UTC.

Change the minutes calculation to allow for time zones that don't use
hour or half hour offsets (e.g. Australia/Eucla's UTC+08:45).

Resolves jekyll#8516.
@ashmaroli
Copy link
Member

@philr Thank you very much for submitting this PR.
Everything LGTM on a quick read-through. If its not asking too much, could you please add a Cucumber based test affirming the non-half hour offset behavior in features/site_configuration.feature.

This module is currently used only for builds on Windows platform. So, does native zoneinfo on non-Windows OS / platforms correctly set non-half hour offset out-of-the-box? If not, could the WinTZ module be re implemented as a platform-agnostic TZ handler? (Mainly asking w.r.t a long-existing issue that got closed due to inactivity; #7550)

@philr
Copy link
Contributor Author

philr commented Nov 16, 2021

I've added a scenario to test the non-half hour offset behaviour.

Native zoneinfo will handle the non-half hour offset correctly.

Copy link
Member

@mattr- mattr- left a comment

Choose a reason for hiding this comment

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

LGTM

@ashmaroli ashmaroli changed the title Support both tzinfo v1 and v2. Support non-half hour offsets. Support both tzinfo v1 and v2 alongwith non-half hour offsets. Nov 17, 2021
@ashmaroli
Copy link
Member

@mattr- Requesting a re-review for this one.
I updated the branch to:

  • use tzinfo-data gem and tzinfo gem only on Windows (and JRuby)
  • additional Appveyor test-matrix entries to explicitly test with tzinfo v1 and tzinfo v2. (Downside being increased CI runtime).

Gemfile Outdated
@@ -32,8 +32,6 @@ group :test do
gem "test-theme", :path => File.expand_path("test/fixtures/test-theme", __dir__)
gem "test-theme-skinny", :path => File.expand_path("test/fixtures/test-theme-skinny", __dir__)
gem "test-theme-symlink", :path => File.expand_path("test/fixtures/test-theme-symlink", __dir__)
gem "tzinfo", ">= 1", "< 3"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved tzinfo into the test group because the new TestWinTz tests run on all platforms. The tests are still passing on non-Windows because tzinfo is being brought in through another dependency.

Copy link
Member

Choose a reason for hiding this comment

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

The WinTZ utility module is only used in Windows:

jekyll/lib/jekyll.rb

Lines 133 to 139 in f51ccbf

def set_timezone(timezone)
ENV["TZ"] = if Utils::Platforms.really_windows?
Utils::WinTZ.calculate(timezone)
else
timezone
end
end

So as of now, testing the module on non-Windows platforms is redundant.
That said, I was hoping to use the WinTZ in all platforms in a separate PR, after merging this.

@ashmaroli
Copy link
Member

Totally forgot about this.
Thank you very much @philr. 馃檹

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 9c9cf3e into jekyll:master Dec 8, 2021
jekyllbot added a commit that referenced this pull request Dec 8, 2021
github-actions bot pushed a commit that referenced this pull request Dec 8, 2021
Phil Ross: Support both tzinfo v1 and v2 alongwith non-half hour offsets. (#8880)

Merge pull request 8880
@jekyll jekyll locked and limited conversation to collaborators Dec 8, 2022
@parkr
Copy link
Member

parkr commented Jan 26, 2023

Backporting to v3.9.x in #9280.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update dependency constraint to allow for tzinfo v2.0.4
5 participants