Mention active_support/time in README.

Rykian / clockwork

A scheduler process to replace cron.

MIT License

544 stars 66 forks source link

Mention active_support/time in README. #40

Closed kenn closed 6 years ago

kenn commented 6 years ago

Breaking change: clockwork stopped working after updating from 2.0.2 to 2.0.3 due to 3.minutes getting NoMethodError, and it turned out, this change caused the issue.

https://github.com/Rykian/clockwork/commit/7418d24026ab4bcad3cd3a19d46d45c61275ac68

I've fixed the issue by adding back 'active_support/time'.

require 'clockwork'
require 'active_support/time'

As we have config examples with 3.minutes or 1.hour in README, I believe we should also have require 'active_support/time' in the examples as well.

Rykian commented 6 years ago

Thanks for the report, it's updated!

javierjulio commented 6 years ago

Thanks @kenn! But why was this just a documentation change @Rykian? I'm disappointed that 7418d24 went out disguised as a patch release as it did cause breakage for Rails apps despite its intention not too. In fact the test in c055d0c clearly gives it away as it had to include active_support/time just for them to pass. That should have been a red flag. I want to help contribute to correct this. Would you be open to:

a PR for reverting commit 7418d24 (and the others in #18) and releasing as 2.0.4? I can prep that with a changelog.
maybe consider pulling down the 2.0.3 release?
another PR to re-introduce commit 7418d24 for a 3.0 major release? I would suggest having better tests too for when that commit is reintroduced. I'm not sure if the change is sufficient to handle timezone correctly.

pboling commented 6 years ago

maybe consider pulling down the 2.0.3 release?

The workaround is not egregious, and many people probably don't even notice the bug, having already loaded the library. Yanking is an earthquake. This is simply a bug like any other, calling for a new patch.

javierjulio commented 6 years ago

@pboling I agree, I think a patch is sufficient. Keep in mind people won't notice because its a silent failure which isn't good.

@Rykian would you be open to it? I can work on that and have it ready for you.

Rykian commented 6 years ago

Yep, I’m open to a patch.

Thank you in advance ;)

javierjulio commented 6 years ago

@Rykian great thanks! I've set aside time tomorrow so I'll work on it then and have it ready for you. I'll make some suggestions too for those that would like to remove the dependency whether as a 3.0 or a 2.x release. I think it could be removed earlier if the library makes it an optional dependency.