After upgrade from 4.0.1 to 5.3.1 option[:vague] raises error #126

mozcomp · 2021-06-18T01:35:12Z

In relation to Issue: #125

Following further tests as to cause of the issue, it relates to the interaction between the optional 3rd argument for "include seconds" and the 4th argument "options". If the 3rd argument was supplied (true/false), then the code worked as expected.
This was included and passed in spec tests.
Adding same specs, but without the optional "include_seconds" argument, gave a series of fails as found in the Issue raised.

My solution may not be acceptable, as I'm changing the arguments supplied, but by testing for a valid true/false of the "include_seconds" argument, and if "options" are nil, then swap the contents.

Once this is done, with the minor tweak of excluding the vague option before passing to the ActiveSupport support version, the code now passes all tests.

dblock · 2021-06-18T04:39:08Z

I think what we really want is a test calling ActionView time_ago_in_words(..., vague: true), that's the only external API. And it is documented as something that accepts include_seconds: true, but nothing else. Finally, in our README we don't even show examples of vague.

So... first question first, how do we want it to work?

mozcomp · 2021-06-18T06:44:48Z

On reading your comment about time_ago_in_words I realised I was getting the same error as @MSCAU but mine was coming from distance_of_time_in_words.

My PR had already skipped a step as to the cause of the problem.

You're correct there are no tests for time_ago_in_words(..., vague: true) but there are for distance_of_time_in_words(..., vague: true) and it's at this point that we get the error.

Our entry point for the error is helper.distance_of_time_in_words and it's here that the code chooses to call the original method aliased as _distance_of_time_in_words or the new DOTIW method.

Looking at the arguments to the method distance_of_time_in_words(from_time, to_time = 0, include_seconds_or_options = {}, options = {}) then at the DOTIW method, you'll see the expected handling of the arguments ...

  def distance_of_time_in_words(from_time, to_time = 0, include_seconds_or_options = {}, options = {})
    raise ArgumentError, "nil can't be converted to a Time value" if from_time.nil? || to_time.nil?
    if include_seconds_or_options.is_a?(Hash)
         options = include_seconds_or_options
     else
         options = options.dup
         options[:include_seconds] ||= !!include_seconds_or_options
      end

But this is done after we have supposedly made use of the vague option and called the original ActionView method.

However, if the include_seconds_or_options argument is the options hash, rather than the include_seconds true/false, two things happen

the ActionView method ISN'T called because the vague option was in include_seconds_or_options not in options
the vague option isn't excluded from the arguments passed to the DOTIW method as expected, so is then passed to the downstream to_sentence where it throws ArgumentError (Unknown key: :vague. Valid keys are: :words_connector, :two_words_connector, :last_word_connector, :locale)

My thoughts are that the argument handling that is done in the DOTIW method, should be moved/duplicated to the ActionView method.

Failing that make the include_seconds argument mandatory thereby avoiding the whole issue.

dblock · 2021-06-18T14:47:05Z

Ok I’m with you @mozcomp.

Before we do this, … we inherited this mess … do you think it would be better to collapse these two arguments (include_seconds_or_options and options) into just options to match time_ago_in_words, and break backwards compatibility in a major version? I think that would fix the confusion once and for all and avoid edge cases. I’d vote to do that.

mozcomp · 2021-06-21T01:17:17Z

@dblock I'm torn, the non-intrusive fix is to copy the existing argument handling within the DOTIW method to the Actionview method, this would make it work as in the original version.

However, as you say, the cleaner solution would be to collapse these arguments into a single argument options.

My main problem with the latter - considering how long it has been since the change that broke the gem (about 12 months I think), there is obviously only a few people implementing the gem with the 3 argument vague option, so if you collapse the options, we might be breaking more than we're fixing.

I'm inclined to vote for the first option rather than the second.

having collapsed arguments, pass options as single hash

dblock · 2021-06-21T14:00:12Z

@mozcomp Ok, I agree we should just fix the problem in the first place

The code looks good. We also need specs for when include_seconds_or_options = true/false here, right?

Please also update https://github.com/radar/distance_of_time_in_words/blob/master/CHANGELOG.md

mozcomp · 2021-06-22T02:52:53Z

Ok I’ll sort the spec tomorrow - away from the office at the moment

…

On Mon, 21 Jun 2021 at 11:30 pm, Daniel Doubrovkine (dB.) < ***@***.***> wrote: @mozcomp <https://github.com/mozcomp> Ok, I agree we should just fix the problem in the first place The code looks good. We also need specs for when include_seconds_or_options = true/false here, right? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#126 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADSWVH5NYDRMZ6GEAL64MTTT5AX7ANCNFSM464XNYEA> .

additional tests include/exclude seconds argument

mozcomp · 2021-07-01T07:44:10Z

Sorry so long getting back to this ...

Added specs to cover all the permutations of the seconds & vague arguments
Updated the readme to be more specific on both the above arguments.

Feel free to change all the above as these are all new for me to be contributing.

dblock · 2021-07-04T16:13:28Z

Sorry so long getting back to this ...

Added specs to cover all the permutations of the seconds & vague arguments
Updated the readme to be more specific on both the above arguments.

Feel free to change all the above as these are all new for me to be contributing.

I think you forgot to push the code up here ;) Thank you!

mozcomp · 2021-07-04T22:59:11Z

That's a rookie error - done now.

dblock · 2021-07-05T18:43:45Z

One more thing, please add a line to https://github.com/radar/distance_of_time_in_words/blob/master/CHANGELOG.md. I think this is good to go otherwise!

dblock

One more thing I spotted.

dblock · 2021-07-05T18:45:01Z

lib/dotiw/action_view/helpers/date_helper.rb

+        if include_seconds_or_options.is_a?(Hash)
+          options = include_seconds_or_options
+        else
+          options[:include_seconds] ||= !!include_seconds_or_options


This modifies the incoming hash, which is bad (TM). Do options = options.merge(...) here.

mozcomp · 2021-07-10T02:47:05Z

I think I have the arguments handling right, but I didn't like that the same code was being repeated for the two calls to the actionview methods, so moved it to a private method

dblock

Some more nitpicks

CHANGELOG.md

dblock · 2021-07-12T14:27:18Z

CHANGELOG.md

@@ -1,6 +1,6 @@
 ## 5.3.2 (Next)

-* Your contribution here.
+* [#126](https://github.com/radar/distance_of_time_in_words/pull/126): Fixes `#distance_of_time_in_words_to_now` with `vague: true` when supplied without include_seconds argument - [@mozcomp](https://github.com/mozcomp)


Quote include_seconds

dblock · 2021-07-12T14:29:04Z

lib/dotiw/action_view/helpers/date_helper.rb

+
+      private
+      def merge_options(include_seconds_or_options, options)
+        merged_options = options.dup


This seems like it's doing more work than needed in all cases, duping and merging. This is simpler:

def merge_options(include_seconds_or_options, options) if include_seconds_or_options.is_a?(Hash) options.merge(include_seconds_or_options) else options.merge(include_seconds: !!include_seconds_or_options) end end

Sorry, newbie error, I thought you were making the change, not waiting for me to do it.
Change as suggested

dblock · 2021-08-16T14:16:06Z

Thank you. See comments above, please make the minor changes to CHANGELOG, and I think this will be good to merge!

dblock · 2021-08-16T14:16:40Z

README.markdown

@@ -65,7 +65,8 @@ The third argument for this method is whether or not to include seconds. By defa
 => "1 year, and 1 second"
 ```

-Yes this could just be merged into the options hash but I'm leaving it here to ensure "backwards-compatibility", because that's just an insanely radical thing to do. \m/
+Yes this could just be merged into the options hash but I'm leaving it here to ensure "backwards-compatibility", because that's just an insanely radical thing to do.  


I think you should put back the original author's \m/ here, keep the spirit of the project alive.

dblock · 2021-08-17T15:09:51Z

CHANGELOG.md

@@ -1,6 +1,7 @@
 ## 5.3.2 (Next)

- * Your contribution here.
+* Your contribution here.
+* [#126](https://github.com/radar/distance_of_time_in_words/pull/126): Fixes `#distance_of_time_in_words_to_now` with `vague: true` when supplied without `include_seconds` argument - [@mozcomp](https://github.com/mozcomp)


You're missing a period at the end. A bot will complain.

dblock · 2021-11-08T19:42:00Z

Release in 5.3.2

mozcomp added 2 commits June 18, 2021 10:36

add spec for excluding 3rd argument 'include secs'

5e16b7f

pass options to 4th argument unless 3rd supplied

52bfae5

mozcomp marked this pull request as ready for review June 18, 2021 01:35

additional test to guard against options already provided

e19819f

re-instate argument handling from DOTIW method

27a5ee4

having collapsed arguments, pass options as single hash

dblock mentioned this pull request Jun 21, 2021

Support #distance_of_time_in_words_to_now with vague: true. #117

Merged

Clarification in readme,

13e3c67

additional tests include/exclude seconds argument

dblock requested changes Jul 5, 2021

View reviewed changes

ensure incoming hash remains unmodified

2b99fff

update changelog

4d41a21

dblock requested changes Jul 12, 2021

View reviewed changes

UPD: option setting - less work per recomender

959f834

dblock reviewed Aug 16, 2021

View reviewed changes

mozcomp added 2 commits August 17, 2021 08:03

Backtrack change to changelog

9b3fae0

Backtrack previous change add quotes

5b9ebef

dblock reviewed Aug 17, 2021

View reviewed changes

Fix: complaining bots ..

30dbfc2

dblock merged commit fec81b2 into radar:master Aug 18, 2021

dblock mentioned this pull request Nov 8, 2021

After upgrading from 4.0.1 to 5.3.1, time_ago_in_words(b.updated_at, vague: true) no longer works #125

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

After upgrade from 4.0.1 to 5.3.1 option[:vague] raises error #126

After upgrade from 4.0.1 to 5.3.1 option[:vague] raises error #126

mozcomp commented Jun 18, 2021

dblock commented Jun 18, 2021

mozcomp commented Jun 18, 2021 •

edited

Loading

dblock commented Jun 18, 2021 •

edited

Loading

mozcomp commented Jun 21, 2021

dblock commented Jun 21, 2021 •

edited

Loading

mozcomp commented Jun 22, 2021 via email

mozcomp commented Jul 1, 2021

dblock commented Jul 4, 2021

mozcomp commented Jul 4, 2021

dblock commented Jul 5, 2021

dblock left a comment

dblock Jul 5, 2021

mozcomp commented Jul 10, 2021

dblock left a comment

dblock Jul 12, 2021

dblock Jul 12, 2021

mozcomp Aug 16, 2021

dblock commented Aug 16, 2021

dblock Aug 16, 2021 •

edited

Loading

dblock Aug 17, 2021

dblock commented Nov 8, 2021

After upgrade from 4.0.1 to 5.3.1 option[:vague] raises error #126

After upgrade from 4.0.1 to 5.3.1 option[:vague] raises error #126

Conversation

mozcomp commented Jun 18, 2021

dblock commented Jun 18, 2021

mozcomp commented Jun 18, 2021 • edited Loading

dblock commented Jun 18, 2021 • edited Loading

mozcomp commented Jun 21, 2021

dblock commented Jun 21, 2021 • edited Loading

mozcomp commented Jun 22, 2021 via email

mozcomp commented Jul 1, 2021

dblock commented Jul 4, 2021

mozcomp commented Jul 4, 2021

dblock commented Jul 5, 2021

dblock left a comment

Choose a reason for hiding this comment

dblock Jul 5, 2021

Choose a reason for hiding this comment

mozcomp commented Jul 10, 2021

dblock left a comment

Choose a reason for hiding this comment

dblock Jul 12, 2021

Choose a reason for hiding this comment

dblock Jul 12, 2021

Choose a reason for hiding this comment

mozcomp Aug 16, 2021

Choose a reason for hiding this comment

dblock commented Aug 16, 2021

dblock Aug 16, 2021 • edited Loading

Choose a reason for hiding this comment

dblock Aug 17, 2021

Choose a reason for hiding this comment

dblock commented Nov 8, 2021

mozcomp commented Jun 18, 2021 •

edited

Loading

dblock commented Jun 18, 2021 •

edited

Loading

dblock commented Jun 21, 2021 •

edited

Loading

dblock Aug 16, 2021 •

edited

Loading