Duration patch - Suggestions for improvements

I wrote a patch which introduced the Duration class to help deal with unifying and making precise the datetime operations in Rails. The major advantage is that things like 1.month.from_now would use Time#advance, using the Duration class to keep state in a similar way to how JavaScriptProxy does. Another benefit is that adding Dates to the result of a call to 5.minutes (etc) will behave as expected.

While I believe the result is something that should be included, I'm less certain about the implementation - I believe it could be written more cleanly. Please see the ticket at http://dev.rubyonrails.org/ticket/6835 and the patch at http://dev.rubyonrails.org/attachment/ticket/6835/add-duration-to-date-time.diff. How would you improve it?

I wrote a patch which introduced the Duration class to help deal with unifying and making precise the datetime operations in Rails. The major advantage is that things like 1.month.from_now would use Time#advance, using the Duration class to keep state in a similar way to how JavaScriptProxy does. Another benefit is that adding Dates to the result of a call to 5.minutes (etc) will behave as expected.

While I believe the result is something that should be included, I'm less certain about the implementation - I believe it could be written more cleanly. Please see the ticket at http://dev.rubyonrails.org/ticket/6835 and the patch at http://dev.rubyonrails.org/attachment/ticket/6835/add-duration-to-date-time.diff. How would you improve it?

There are some other cases which need to work:

some_time + 2.days - 3.months some_time + 2.months + 1.day

However if you can get all this going, then I'm definitely interested in merging something like this to trunk.

Okay, I just added another patch to the ticket that includes the test cases you mentioned Koz.

Nice work, further thoughts. Perhaps Duration should sit in ActiveSupport:: rather than hidden away in the core extensions?

Why do you use the blankslate?

Other than that, it's looking good.

Nice work, further thoughts. Perhaps Duration should sit in ActiveSupport:: rather than hidden away in the core extensions?

Sure. I just put it there since I didn't envision people using it directly and it was just simpler to write it that way.

Why do you use the blankslate?

I use blankslate in order to maintain backward compatibility with existing applications that may depend on the fact that 1.hour returns a Numeric type. So Duration acts like a Numeric when it is treated as one (for example, when using < and >). This compatibility is not actually required for any of my applications, but I figured that it would be for others.

Unless you have any other suggestions, I'll create another patch with duration living directly under ActiveSupport. Would you suggest giving it its own file?

Patch has been updated with Duration directly under ActiveSupport, plus a test for Duration#inspect and some documentation fixes. Let me know if anything else needs to be done.

Glad the patch was finally accepted! Thanks to nzkoz for overseeing it and to minam for making it duck-safe.