distance_of_time_in_words / time_ago_in_words shouldn't use named arguments #898

Closed
lighthouse-import opened this Issue May 16, 2011 · 5 comments

Comments

Projects
None yet
1 participant
@lighthouse-import

Imported from Lighthouse. Original ticket at: http://rails.lighthouseapp.com/projects/8994/tickets/6522
Created by Clemens Kofler - 2011-03-04 16:45:14 UTC

In contrast to most other view helpers, distance_of_time_in_words and time_ago_in_words still use a named argument (include_seconds). I propose to get things in line with the rest and pass :include_seconds => true/false as part of an options hash.

# old
distance_of_time_in_words(19.seconds.ago, Time.current, true) # => less than 20 seconds
time_ago_in_words(19.seconds.ago, true) # => less than 20 seconds

# new
distance_of_time_in_words(19.seconds.ago, Time.current, :include_seconds => true) # => less than 20 seconds
time_ago_in_words(19.seconds.ago, :include_seconds => true) # => less than 20 seconds

Although there's a little more typing, I'd argue that it's less surprising.

A patch is attached. All tests are green and the patch also adds new tests (same as the old ones with just the argument swapped out for a hash). I've also updated the documentation and added a deprecation warning.

@lighthouse-import

This comment has been minimized.

Show comment Hide comment
@lighthouse-import

lighthouse-import May 16, 2011

Imported from Lighthouse.
Comment by Dan Pickett - 2011-03-13 17:32:16 UTC

I'm +1 on this and it applies cleanly to master, but it creates a lot of noise in the test suite currently. Can you update the test suite to comply with your new contract?

Imported from Lighthouse.
Comment by Dan Pickett - 2011-03-13 17:32:16 UTC

I'm +1 on this and it applies cleanly to master, but it creates a lot of noise in the test suite currently. Can you update the test suite to comply with your new contract?

@lighthouse-import

This comment has been minimized.

Show comment Hide comment
@lighthouse-import

lighthouse-import May 16, 2011

Imported from Lighthouse.
Comment by Clemens Kofler - 2011-03-13 21:24:36 UTC

Sure – I just left the old tests untouched to ensure things still work. They are removed in the attached patch. I've also rebased against the current master.

Imported from Lighthouse.
Comment by Clemens Kofler - 2011-03-13 21:24:36 UTC

Sure – I just left the old tests untouched to ensure things still work. They are removed in the attached patch. I've also rebased against the current master.

@lighthouse-import

This comment has been minimized.

Show comment Hide comment
@lighthouse-import

lighthouse-import May 16, 2011

Imported from Lighthouse.
Comment by Dan Pickett - 2011-03-14 16:09:46 UTC

a model patch. Applies cleanly to master and appears well documented.

Thanks Clemens. Assigning to Santiago for his review and signoff.

Imported from Lighthouse.
Comment by Dan Pickett - 2011-03-14 16:09:46 UTC

a model patch. Applies cleanly to master and appears well documented.

Thanks Clemens. Assigning to Santiago for his review and signoff.

@lighthouse-import

This comment has been minimized.

Show comment Hide comment
@lighthouse-import

lighthouse-import May 16, 2011

Imported from Lighthouse.
Comment by Clemens Kofler - 2011-05-02 14:57:22 UTC

Any news on that one? Not that there's any time pressure on it but I don't want it to become stale. :)

Imported from Lighthouse.
Comment by Clemens Kofler - 2011-05-02 14:57:22 UTC

Any news on that one? Not that there's any time pressure on it but I don't want it to become stale. :)

@lighthouse-import

This comment has been minimized.

Show comment Hide comment
@lighthouse-import

lighthouse-import May 16, 2011

Attachments saved to Gist: http://gist.github.com/971801

Attachments saved to Gist: http://gist.github.com/971801

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment