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
Explicitly require AS::Time in AS::Testing::TimeHelpers #28790
Explicitly require AS::Time in AS::Testing::TimeHelpers #28790
Conversation
r? @pixeltrix (@rails-bot has picked a reviewer for you, use r? to override) |
@pixeltrix I know you just reviewed/merged #28788 as well — I swear I'm not just trawling the codebase looking for these 😜 These two specifically are libs that I wanted to include standalone in a smaller project — I think they're good candidates for that in general. I promise I don't have 40 of these in the hopper behind this one. |
@@ -1,4 +1,5 @@ | |||
require "active_support/core_ext/string/strip" # for strip_heredoc | |||
require "active_support/time" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we require only what we need. I believe only active_support/core_ext/time/calculations
should be sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rafaelfranca Updated.
If you just try to use `ActiveSupport::Testing::TimeHelpers` standalone by requiring `active_support/testing/time_helpers`, you currently get an error: `NoMethodError: undefined method `change' for 2017-12-14 01:04:44 -0500:Time` 9f6e82e added a dependency on `AS::Time` by using `AS::Time#change`. Here's a script to reproduce the error: ```ruby require "bundler/inline" gemfile(true) do source "https://rubygems.org" gem "activesupport", github: "rails/rails" end require "active_support/testing/time_helpers" require "minitest/autorun" class BugTest < Minitest::Test include ActiveSupport::Testing::TimeHelpers def test_stuff travel_to Time.new(2017, 12, 14, 01, 04, 44) do assert true end end end ``` It currently fails for all 5.x.x versions and master. Ideally, this would be backported to `5-0-stable` and `5-1-stable` as well.
119feb1
to
1d82b7c
Compare
@rafaelfranca And a note re: #28788 (comment), this PR's repro script above also fails when you include a top-level |
👍 I double checked that before making the comment. |
…me_helpers Explicitly require AS::Time in AS::Testing::TimeHelpers
…me_helpers Explicitly require AS::Time in AS::Testing::TimeHelpers
If you just try to use
ActiveSupport::Testing::TimeHelpers
standalone by requiringactive_support/testing/time_helpers
, you currently get an error:NoMethodError: undefined method `change' for 2017-12-14 01:04:44 -0500:Time
.9f6e82e added a dependency on
AS::Time
by usingAS::Time#change
.Here's a script to reproduce the error:
It currently fails for all 5.x.x versions and master. Ideally, this would be backported to
5-0-stable
and5-1-stable
as well.Thanks!