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.

Sign up for GitHub

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

Jump to bottom

Time#change should pass the zone property through if it's set #42583

Merged
merged 1 commit into from Jun 26, 2021
Merged

Time#change should pass the zone property through if it's set #42583

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions activesupport/CHANGELOG.md
View file
Open in desktop
@@ -1,3 +1,11 @@
* `Time#change` and methods that call it (eg. `Time#advance`) will now
return a `Time` with the timezone argument provided, if the caller was
initialized with a timezone argument.

Fixes [#42467](https://github.com/rails/rails/issues/42467).

*Alex Ghiculescu*

* Allow serializing any module or class to JSON by name

*Tyler Rick*, *Zachary Scott*
Expand Down
2 changes: 2 additions & 0 deletions activesupport/lib/active_support/core_ext/time/calculations.rb
View file
Open in desktop
Expand Up @@ -159,6 +159,8 @@ def change(options)
::Time.new(new_year, new_month, new_day, new_hour, new_min, new_sec, new_offset)
elsif utc?
::Time.utc(new_year, new_month, new_day, new_hour, new_min, new_sec)
elsif zone&.respond_to?(:utc_to_local)
::Time.new(new_year, new_month, new_day, new_hour, new_min, new_sec, zone)
Copy link
Member Author

Choose a reason for hiding this comment

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

my first attempt at a fix merged this branch and the next one, but Time.new doesn't like zones formatted as some kinds of strings, eg. it was crashing on ::Time.new(new_year, new_month, new_day, new_hour, new_min, new_sec, "CST")

Copy link
Contributor

Choose a reason for hiding this comment

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

time.zone isn't necessarily an instance of ActiveSupport::TimeWithZone - Ruby allows any object there as long as it quacks in the correct manner. Perhaps a zone.respond_to?(:utc_to_local) would be better?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's the docs I was trying to find! I'll tidy it up, thanks.

elsif zone
::Time.local(new_year, new_month, new_day, new_hour, new_min, new_sec)
else
Expand Down
8 changes: 8 additions & 0 deletions activesupport/test/core_ext/time_ext_test.rb
View file
Open in desktop
Expand Up @@ -483,6 +483,14 @@ def test_advance
assert_equal Time.local(2005, 2, 28, 20, 22, 19), Time.local(2005, 2, 28, 15, 15, 10).advance(hours: 5, minutes: 7, seconds: 9)
assert_equal Time.local(2005, 2, 28, 10, 8, 1), Time.local(2005, 2, 28, 15, 15, 10).advance(hours: -5, minutes: -7, seconds: -9)
assert_equal Time.local(2013, 10, 17, 20, 22, 19), Time.local(2005, 2, 28, 15, 15, 10).advance(years: 7, months: 19, weeks: 2, days: 5, hours: 5, minutes: 7, seconds: 9)

assert_equal Time.new(2021, 5, 29, 0, 0, 0, "+03:00"), Time.new(2021, 5, 29, 0, 0, 0, ActiveSupport::TimeZone["Moscow"])
assert_equal Time.new(2021, 5, 29, 0, 0, 0, "+03:00").advance(seconds: 60), Time.new(2021, 5, 29, 0, 0, 0, ActiveSupport::TimeZone["Moscow"]).advance(seconds: 60)
assert_equal Time.new(2021, 5, 29, 0, 0, 0, "+03:00").advance(days: 3), Time.new(2021, 5, 29, 0, 0, 0, ActiveSupport::TimeZone["Moscow"]).advance(days: 3)

assert_equal Time.new(2021, 5, 29, 0, 0, 0, "+03:00"), ActiveSupport::TimeZone["Moscow"].local(2021, 5, 29, 0, 0, 0)
assert_equal Time.new(2021, 5, 29, 0, 0, 0, "+03:00").advance(seconds: 60), ActiveSupport::TimeZone["Moscow"].local(2021, 5, 29, 0, 0, 0).advance(seconds: 60)
assert_equal Time.new(2021, 5, 29, 0, 0, 0, "+03:00").advance(days: 3), ActiveSupport::TimeZone["Moscow"].local(2021, 5, 29, 0, 0, 0).advance(days: 3)
end

def test_utc_advance
Expand Down