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

Compute time zones on all platforms with tzinfo gem #7551

Closed
wants to merge 8 commits into from

Conversation

ashmaroli
Copy link
Member

@ashmaroli ashmaroli commented Feb 26, 2019

  • This is a 馃檵 feature or enhancement.
  • I've added tests

Summary

Calculate Time Zone with tzinfo library on all platforms

  • Reverts Upgrade WinTZ utility to use TZInfo-2.0聽#7521 and adds tzinfo 1.x as a runtime_dependency of Jekyll
    The revert is because activesupport gem currently locks to tzinfo 1.x. Even though Jekyll has no direct dependency on activesupport, there may be plugins out there that depend on a recent version of ActiveSupport.
  • Renames Utils::WinTZ module as Utils::TimeZone

Context

Resolves #7550

@update-docs

This comment has been minimized.

* Add tzinfo-data as dependency
* Add tzinfo 1.x as dependency
  Since 'activesupport' gem currently locks to tzinfo-1.x, we're locking
  to that as well to avoid conflicts with plugins that use ActiveSupport
@ashmaroli ashmaroli changed the title Test parsing date with zone in post's front matter WIP: Fix parsing date with zone in post's front matter Feb 26, 2019
@ashmaroli ashmaroli changed the title WIP: Fix parsing date with zone in post's front matter Compute time zones on all platforms with tzinfo gem Feb 26, 2019
@ashmaroli ashmaroli added this to the 4.0 milestone Feb 26, 2019
@ashmaroli ashmaroli requested a review from a team February 26, 2019 21:30
@mattr-
Copy link
Member

mattr- commented Feb 26, 2019

Does this do anything to resolve #5963?

@ashmaroli
Copy link
Member Author

Does this do anything to resolve #5963?

@mattr- Honestly, I did not even realize that a similar issue was previously reported. The current pull request is a direct reaction to the failing cucumber test in 4e68111 in this pull request.
(Notably, the tests fail on Travis on all tested Ruby versions but not on AppVeyor)

I'll try and test further to check if this issue is a regression due to cf5c689 shipped in Jekyll-3.7.1

The ENV["TZ"] *ought to* be correctly set during the course of
a `jekyll build` session
@ashmaroli
Copy link
Member Author

@mattr- From my investigation, the inference is that #5963 was resolved by #6697.
This PR attempts to clear out an aspect overlooked in #6697: post data with date, time and time zone.
I feel this issue was overlooked in the tests because the scenarios were set to UTC time zone.

Disclaimer: I'm not currently sure if the tests here will fail when changes due to DST kick in..

@ashmaroli ashmaroli modified the milestones: 4.0, 4.1 May 3, 2019
@DirtyF DirtyF added this to In progress in Jekyll 4.1 Aug 14, 2019
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.

Let's get the conflicts resolved and CI passing so this can be merged.

@ashmaroli ashmaroli removed this from the 4.1 milestone May 6, 2020
@ashmaroli ashmaroli removed this from In progress in Jekyll 4.1 May 6, 2020
@scottdorman
Copy link

What will it take to move this PR forward and get it merged (besides the obvious of fixing the merge conflicts)? It's been over a year since the PR was submitted and about 6 months since the changes in the PR were approved.

@ashmaroli
Copy link
Member Author

@scottdorman Sorry for the delay, but technically, we're sort of at a stalemate here.
The tests show that the behavior is flaky and changes with DST changes.

And then there's the issue with dependencies.. The current timezone management in Jekyll requires the gem tzinfo-1.x but then that would cause issues with an indirect dependency gem named activesupport whose newer versions do not support
tzinfo-1.x...

So in other words, its a messy situation..
Was hoping if someone from the community could propose a better solution..

@scottdorman
Copy link

@ashmaroli Thanks for the update. On the surface, it seems like such a simple thing to fix. :) It seems like doing something around using UTC dates here would be the solution, or at least part of the solution, since that should remove the issues around DST changes.

@ashmaroli ashmaroli closed this Aug 26, 2021
@ashmaroli ashmaroli deleted the parse-date-timezone-test branch August 26, 2021 05:34
@jekyll jekyll locked and limited conversation to collaborators Aug 26, 2022
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.

The wrong date can be used in the url
5 participants