Ensure input to distance_of_time_in_words is not nil

[iamvery]

Problem

The error message for bad input to the distance_of_time_in_words and related methods is cryptic. For example, if nil is provided as an argument to time_ago_in_words, this is the error:

irb> helper.time_ago_in_words(nil)
NoMethodError: undefined method `>' for nil:NilClass

I believe a more palatable result in this case would be to raise an ArgumentError for invalid input.

(Proposed) Solution

The provided implementation is a bit naive in that it only checks for nil arguments. While investigating the problem, I realized there are several different ways in which the input may be "bad".

1. Argument is nil

2. The from_time and to_time arguments are incompatible with one another. e.g.

   irb> helper.distance_of_time_in_words(Date.current, 0)
   ArgumentError: comparison of Time with 0 failed

3. moar?

Generally it seems like the interface requires either two numeric arguments, or two to_time convertible arguments.

To that end, it felt like this issue may benefit from the input of parties more familiar with the code. I would be honored and willing to help with the solution. If I am able to come up with a more "elegant" solution to the type situation, I will gladly post it back here.

Thank you! 💫

---

TODO

• settle on solution
• update docs (if needed)
• update changelog

[iamvery]

Perhaps converting input to Numeric before any of the comparisons to avoid the mixed type case. Would that be weird? So assuming before this line there was something like:

from_time, to_time = from_time.to_f, to_time.to_f

You could do things like this, where it would normally be a comparison error:

irb> helper.distance_of_time_in_words(Time.now)
=> "over 45 years"
irb> helper.distance_of_time_in_words(0, Time.now)
=> "over 45 years"
irb> helper.distance_of_time_in_words(Time.now, 0)
=> "over 45 years"

Is that sane?

[iamvery]

@sgrif is there anything I can do to improve this so that it gets some attention?

[gsamokovarov]

Rails gets quite a lot of issues and pull requests. Give it a bit of time, people will catch up on the issue. Thank you so much for the work!

[rafaelfranca]

Maybe changing:

from_time = from_time.to_time if from_time.respond_to?(:to_time)
to_time = to_time.to_time if to_time.respond_to?(:to_time)

to:

if from_time.respond_to?(:to_time)
  from_time = from_time.to_time
else
  from_time = from_time.to_f
end

if to_time.respond_to?(:to_time)
  to_time = to_time.to_time
else
  to_time = to_time.to_f
end

[iamvery]

@gsamokovarov thanks for keeping me accountable! I can't imagine how overwhelming it must be to see so much activity on a project 😳

@rafaelfranca I believe that's still problematic, because the caller could pass one argument that is convertible to_time and another that is not. That would cause the ArgumentError: comparison of Time with ? failed I mentioned above.

Fundamentally I think the problem is to do with the method signature being so flexible (accepting both time and numeric values). I wonder if it'd be sane to convert all arguments to a common type (e.g. float) to deal with them uniformly inside the method?

[iamvery]

I believe this is effectively a conversion of input to Time. One thing to note is that it's not timezone aware. The suite seemed happy locally so maybe it's not a concern since this method deals in relative times? Truth is I don't trust myself with time, so I'd love a sanity check on this...

[rafaelfranca]

@pixeltrix WDYT?

[pixeltrix]

Should be fine - the two lines above convert any AS::TimeWithZone instances to local times so Time.at just essentially copies the values to a new Time instance. If there was a problem it's probably an underlying bug in to_time.

Thinking about it, maybe it might make things simpler if we define Numeric#to_time ? e.g:

class Numeric
  def to_time
    Time.at(self)
  end
end

This way we can eliminate the two Time.at calls, add a check for whether the two arguments respond to to_time and raise if they don't, eliminating the nil check.

[pixeltrix]

By adding Numeric#to_time you could then make the method setup something like this:

def distance_of_time_in_words(from, to = 0, options = {})
  unless from.respond_to?(:to_time)
    raise ArgumentError, "from must be a time-like value"
  end

  unless to.respond_to?(:to_time)
    raise ArgumentError, "to must be a time-like value"
  end

  from_time, to_time = [from.to_time, to.to_time].sort
  distance_in_minutes = ((to_time - from_time)/60.0).round
  distance_in_seconds = (to_time - from_time).round

the exception messages could be better so feel free to change them

[iamvery]

Very interesting idea! Would there be any concern that the Time conversion of Numeric values is always in local time? Is that confusing*?

* time is always so confusing 😞

[iamvery]

Is there any concern that Numeric conversions to Time are always left in local (system) time?

[matthewd]

@pixeltrix I think I feel like the numeric -> time conversion is too special-case to warrant making all numerics respond to to_time, even though it's still an explicit conversion. 😕

I guess my contention is that the choice of time unit & epoch are both domain concerns of Time, so it feels a stretch to leak them to Numeric. (Which I think might be more obvious once we expand this method's documentation to a workable level of detail.)

[iamvery]

Yeah, I was afraid of this. Shall I throw away those commits and stick with the implementation up to ec2e09b?

[pixeltrix]

@matthewd we support to_time on String which can be pretty much anything - in comparison to_time on Numeric is positively well defined. 😄

@iamvery to_time always returns a local time in the other places it's defined - in_time_zone is for AS::TWZ.

[iamvery]

😂 it's settled then!

[sgrif]

Can we be a bit more explicit with these? It took me a minute to figure out what fyear meant.

[iamvery]

done! :)

[iamvery]

I run into this issue every time I'm teaching our Ruby course. I was reminded this week that the PR is still open. Any chance it can get some attention? Thanks! ❤️

[iamvery]

On a wave of inspiration from RailsConf, I got this PR freshly rebased! Would love to talk about getting it merged. Anyone want to hash it out in person? 💫

[pixeltrix]

Discussed with @iamvery at RailsConf that implement to_time on Numeric is too risky so keeping any type conversion limited to the distance_of_time_in_words helper is key.

[iamvery]

@pixeltrix updated!

[iamvery]

There's a failure for Ruby 2.2.7, but it doesn't appear to be related?

[pixeltrix]

Seems like we could move this up to the start of the method and combine into if statements:

if from_time.is_a?(Numeric)
  from_time = Time.at(from_time)
elsif from_time.respond_to?(:to_time)
  from_time = from_time.to_time
else
  raise ArgumentError, "first argument must either be a time, date or numeric, was '#{from_time.inspect}'"
end

if to_time.is_a?(Numeric)
  to_time = Time.at(to_time)
elsif from_time.respond_to?(:to_time)
  to_time = to_time.to_time
else
  raise ArgumentError, "second argument must either be a time, date or numeric, was '#{to_time.inspect}'"
end

[iamvery]

I'd love to extract a small, private interface to perform this logic. Something like:

def normalize_to_time(value)
  if value.is_a?(Numeric)
    Time.at(value)
  elsif value.responds_to?(:to_time)
    value.to_time
  else
    yield(value) if block_given?
  end
end

Then we could replace these two similar blocks of code with something like:

from_time = normalize_to_time(from_time) { raise ArgumentError, "first argument must be convertible_to_time" }
to_time = normalize_to_time(to_time) { raise ArgumentError, "second argument must be convertible_to_time" }

The only reason that I didn't do that here is I'm not completely certain how Rails uses this module. If I put that in a private method, would it be accessible in the places where this module is included?

Anyway, that feels like it'd be a valuable change, but in the end I decided it might be better to discuss than me stumbling around trying to figure it out.

Thanks for the feedback in helping get this in!

[iamvery]

I wasn't absolutely certain, so I was able to confirm that methods included from a module have access to private interfaces (I suppose they're also included?)

module Lol
  def wat
    haha
  end
  private
  def haha
    "OK"
  end
end

class Giggle
  include Lol
end

Giggle.new
=> #<Giggle:0x007ff69e0a1738>
Giggle.new.wat
=> "OK"

[pixeltrix]

@iamvery yes, that seems fine - I think the method name needs to be more specific to distance_of_time_in_words though as normalize_to_time seems quite generic.

[pixeltrix]

Once you've fixed the two items it'll be good to merge so can you squash your commits, thanks 👍

[pixeltrix]

Can you move these lines down underneath the options

[pixeltrix]

This needs to be private so you'll need to add private and then indent again or rubocop will complain

[iamvery]

🤦‍♂️

[iamvery]

Is there a reason to mutate the new hash rather than doing options.merge(scope: :'datetime.distance_in_words')? I don't particularly care, just curious.

[pixeltrix]

Probably performance reasons

[iamvery]

@pixeltrix updated! do you want/need me to squash anything?

[pixeltrix]

you need to indent this otherwise code climate won't pass

[iamvery]

Ah hah, I'm not used to that style. Will fix and squash!

[pixeltrix]

@iamvery yes, just the indentation fix and squash your commits please 👍

[iamvery]

@pixeltrix rebased the history into a more cohesive story. Let me know if you need anything else. Thanks for sticking with me through this! ❤️

[pixeltrix]

This needs to be raise ArgumentError, "#{value} must be convertible to_time" - getting a NameError when running the tests.

[iamvery]

😂 you'd think I'm tired or something

[pixeltrix]

@iamvery can you squash to a single commit, please. For small changes like this it's cleaner that multiple commits if we need to revert, etc. and we can get the story you're telling from the PR. If we didn't do this it'd make navigating through git history much more painful when trying to do git bisect to find where a bug started, thanks.

[iamvery]

@pixeltrix you have been tremendously patient and helpful. Thank you for all your work to make and keep Rails great.

[pixeltrix]

One last change, I promise 😄

Using #{value} with a nil value isn't helpful, e.g:

>> value = nil
>> "#{value} can't be converted to a Time value"
=> " can't be converted to a Time value"

It's better to use inspect:

>> value = nil
>> "#{value.inspect} can't be converted to a Time value"
=> "nil can't be converted to a Time value"

Can you also tweak the message to what I've used in this comment which is more in line with other similar Ruby messages, thanks.

[iamvery]

No worries at all! I'm honored to have gotten so much great feedback this week 💫 Updated 👊

[pixeltrix]

@iamvery thanks for helping make Rails better 👍

[iamvery]

👏 and you