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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raise ArgumentError when an invalid form is passed to Date#to_time #24552

Merged
merged 1 commit into from
Apr 19, 2016

Conversation

yui-knk
Copy link
Contributor

@yui-knk yui-knk commented Apr 14, 2016

Before this commit
NoMethodError: undefined methodform_name' for Time:Classis raised when an invalid argument is passed. It is better to raiseArgumentError` and show list of valid arguments
to developers.

@rails-bot
Copy link

r? @pixeltrix

(@rails-bot has picked a reviewer for you, use r? to override)

@@ -80,6 +80,7 @@ def readable_inspect
#
# date.to_time(:utc) # => 2007-11-10 00:00:00 UTC
def to_time(form = :local)
raise ArgumentError, "Expected :local or :uts, got #{form}." unless [:local, :utc].include?(form)
Copy link
Member

Choose a reason for hiding this comment

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

:uts typo

Copy link
Member

Choose a reason for hiding this comment

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

form.inspect so we can see whether it's a string or symbol

Copy link
Member

Choose a reason for hiding this comment

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

Can we speed this up (use a case; when) or defer it (rescue NoMethodError and re-raise ArgumentError)?

Before this commit
`NoMethodError: undefined method `form_name' for Time:Class` is raised
when an invalid argument is passed.
It is better to raise `ArgumentError` and show list of valid arguments
to developers.
@yui-knk
Copy link
Contributor Author

yui-knk commented Apr 16, 2016

@jeremy Thanks for your review. I updated 😍

@yui-knk
Copy link
Contributor Author

yui-knk commented Apr 19, 2016

I took benchmark.
Add these methods to activesupport/lib/active_support/core_ext/date/conversions.rb

  def to_time_with_case(form = :local)
    case
    when ![:local, :utc].include?(form)
      raise ArgumentError, "Expected :local or :utc, got #{form.inspect}."
    else
      ::Time.send(form, year, month, day)
    end
  end

  def to_time_with_rescue(form = :local)
    ::Time.send(form, year, month, day)
  rescue NoMethodError
    raise ArgumentError, "Expected :local or :utc, got #{form.inspect}."
  end

and create activesupport/lib/active_support/core_ext/date/conversions.rb

require 'abstract_unit'
require 'active_support/time'
require 'core_ext/date_and_time_behavior'
require 'time_zone_test_helpers'
require 'benchmark/ips'

Benchmark.ips do |x|
  x.report('to_time')             { Date.new(2005, 2, 21).to_time(:utc) }
  x.report('to_time_with_case')   { Date.new(2005, 2, 21).to_time_with_case(:utc) }
  x.report('to_time_with_rescue') { Date.new(2005, 2, 21).to_time_with_rescue(:utc) }
end
$ bundle exec ruby -Itest test/core_ext/date_ext_bench_test.rb
Calculating -------------------------------------
             to_time    30.125k i/100ms
   to_time_with_case    30.181k i/100ms
 to_time_with_rescue    31.234k i/100ms
-------------------------------------------------
             to_time    389.934k (± 4.4%) i/s -      1.958M
   to_time_with_case    367.383k (± 6.7%) i/s -      1.841M
 to_time_with_rescue    391.123k (± 7.9%) i/s -      1.968M
Run options: --seed 15798

# Running:



Finished in 0.000813s, 0.0000 runs/s, 0.0000 assertions/s.

0 runs, 0 assertions, 0 failures, 0 errors, 0 skips

Although to_time_with_rescue is a bit faster than to_time, but I like to_time because it is natural checking arguments at first of method.

@jeremy jeremy merged commit 29e876f into rails:master Apr 19, 2016
jeremy added a commit that referenced this pull request Apr 19, 2016
Raise `ArgumentError` when an invalid form is passed to `Date#to_time`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants