Skip to content

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

Merged
jekyllbot merged 3 commits intojekyll:masterfrom
philr:support_tzinfo_v2
Dec 8, 2021
Merged

Support both tzinfo v1 and v2 alongwith non-half hour offsets.#8880
jekyllbot merged 3 commits intojekyll:masterfrom
philr:support_tzinfo_v2

Conversation

@philr
Copy link
Copy Markdown
Contributor

@philr philr commented Nov 14, 2021 •
edited
Loading

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.

@philr
Support both tzinfo v1 and v2. Support non-half hour offsets.
04ad35c 
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.
@philr philr mentioned this pull request Nov 14, 2021
Closed
@ashmaroli
Copy link
Copy Markdown
Member

ashmaroli commented Nov 15, 2021

@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
Copy Markdown
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.

mattr-
mattr- approved these changes Nov 17, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

ashmaroli commented Nov 17, 2021

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

@ashmaroli ashmaroli requested a review from mattr- November 17, 2021 13:07
philr
philr commented Nov 17, 2021
View reviewed changes
Gemfile Outdated
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
Copy Markdown
Contributor Author

@philr philr Nov 17, 2021

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
Copy Markdown
Member

@ashmaroli ashmaroli Nov 17, 2021

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
Copy Markdown
Member

ashmaroli commented Dec 8, 2021

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
@jekyllbot
Update history to reflect merge of #8880 [ci skip]
fafcc2c
github-actions bot pushed a commit that referenced this pull request Dec 8, 2021
@jekyllbot
Deploy docs from 9c9cf3e
80e09ec 
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
Copy Markdown
Member

parkr commented Jan 26, 2023

Backporting to v3.9.x in #9280.

@philr philr deleted the support_tzinfo_v2 branch December 31, 2025 16:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ashmaroli ashmaroli ashmaroli left review comments

@mattr- mattr- mattr- approved these changes

Assignees

No one assigned

Labels

enhancement frozen-due-to-age

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Update dependency constraint to allow for tzinfo v2.0.4

5 participants

@philr @ashmaroli @parkr @mattr- @jekyllbot