phoffer
commented
6 years ago Hi @javierjulio, could you provide some further detail as to how the change broke Rails apps? I'm the original PR author and would like to fix any issue that popped up.
Some background info regarding the initial PR:
The only reason the tests require active_support is because they frequently utilize 2.minutes style helpers, and I wanted to leave the readability of that. That is why it became a development dependency.
Also, the only tests hitting the changed functionality pass when run on their own, without having active_support available.
I'd be happy to look into whatever issue arose, since our Rails app did not suffer any problems. I would just like some details on what it may be.
tyleriguchi
The changes in #18 were made with the notion of not breaking Rails apps but it ended up doing so. In fact the test in c055d0c gives it away as it had to include
active_support/timejust for them to pass. That should have been a red flag. This reverts the changes from #18 so we can address the ActiveSupport dependency in an appropriate release with tests.Some suggestions for that future update would include:
activesupport/timeis required and used over the default introduced in #18 so both can be utilized and introduced earlier in an 2.x release.In the meantime @Rykian this is meant to address the breaking change as part of a patch release. I included a changelog but in case you'd rather not have it I left it as a separate commit so feel free to remove it. I can also set a date and update the version file too (2.0.4?) to prep for it release if that helps. Let me know. Thanks.