New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use directly fugit instead of rufus-scheduler #200

Merged
merged 7 commits into from Jul 9, 2018

Conversation

Projects
None yet
5 participants
@jmettraux
Contributor

jmettraux commented May 18, 2018

Addresses #199

Sidekiq-cron is not using rufus-scheduler itself but rather its cron logic which recently got spun off in the fugit gem (https://github.com/floraison/fugit).

This patch makes Sidekiq-cron depend directly on fugit instead of rufus-scheduler.

A refinement would be to cache the parsed cron line in the Sidekiq-cron Job instance so that there is no need to reparse each time.

Here is the rake test output after patching. Please note that I did not address the remaining five errors, they seem unrelated, out of the scope for this patch : test1.txt

Use directly fugit instead of rufus-scheduler
Addresses #199

Sidekiq-cron is not using rufus-scheduler itself but rather its cron logic which recently got spun off in the fugit gem (https://github.com/floraison/fugit).

This patch makes Sidekiq-cron depend directly on fugit instead of rufus-scheduler.

A refinement would be to cache the parsed cron line in the Sidekiq-cron Job instance so that there is no need to reparse each time.
@coveralls

This comment has been minimized.

coveralls commented May 18, 2018

Coverage Status

Coverage increased (+0.2%) to 91.711% when pulling 52aa754 on jmettraux:fugit_1.1 into 40f179f on ondrejbartas:master.

@coveralls

This comment has been minimized.

coveralls commented May 18, 2018

Coverage Status

Coverage increased (+0.2%) to 91.667% when pulling ae9701b on jmettraux:fugit_1.1 into 40f179f on ondrejbartas:master.

@Supernats

Thanks for opening this up, @jmettraux. We were also recently surprised by the sudden loss of compatibility and would love to see sidekiq-cron move back to stability.

The core of your change that swaps rufus-scheduler logic with fugit feels muddied by some changes you've included that are unrelated. Would you mind pulling those changes back? I respect that you feel strongly about your chosen styles, but I imagine it's just as like @ondrejbartas feels the same as theirs. Seeing as we are in their space, it seems respectful to keep our changes constrained to the topics at hand.

end
end
errors << "'klass' (or class) must be set" unless klass_valid
!errors.any?
errors.empty?

This comment has been minimized.

@Supernats

Supernats Jun 6, 2018

While I agree positive predicates are preferred, this change feels unnecessary and muddies the diff.

@@ -5,15 +5,15 @@
# stub: sidekiq-cron 0.6.3 ruby lib
Gem::Specification.new do |s|
s.name = "sidekiq-cron"
s.name = "sidekiq-cron".freeze

This comment has been minimized.

@Supernats

Supernats Jun 6, 2018

Freezing string literals seems irrelevant to the task at hand here. I respect that it's a decision you would make for your code, but I would stress respecting @ondrejbartas's ability to write code as they see fit.

@@ -37,7 +37,9 @@ Gem::Specification.new do |s|
"lib/sidekiq/cron/launcher.rb",
"lib/sidekiq/cron/locales/de.yml",
"lib/sidekiq/cron/locales/en.yml",
"lib/sidekiq/cron/locales/ja.yml",

This comment has been minimized.

@Supernats

Supernats Jun 6, 2018

I don't see this file added as part of the diff.

This comment has been minimized.

@ondrejbartas

ondrejbartas Jul 9, 2018

Owner

maybe this wasn't added because of publish

"lib/sidekiq/cron/locales/ru.yml",
"lib/sidekiq/cron/locales/zh-CN.yml",

This comment has been minimized.

@Supernats

Supernats Jun 6, 2018

I don't see this file added as part of the diff.

s.summary = "Sidekiq Cron helps to add repeated scheduled jobs"
s.homepage = "http://github.com/ondrejbartas/sidekiq-cron".freeze
s.licenses = ["MIT".freeze]
s.rubygems_version = "2.6.13".freeze

This comment has been minimized.

@Supernats

Supernats Jun 6, 2018

Why do we need to change the rubygems version?

@@ -391,29 +391,23 @@ def errors
end
def valid?
#clear previos errors
#clear previous errors

This comment has been minimized.

@Supernats

Supernats Jun 6, 2018

Spelling changes are great, but maybe they would be easier to merge in their own PR?

@jmettraux

This comment has been minimized.

Contributor

jmettraux commented Jun 6, 2018

Good morning Nathan,

The core of your change that swaps rufus-scheduler logic with fugit feels muddied by some changes you've included that are unrelated.

OK, I contribute mud.

We were also recently surprised by the sudden loss of compatibility and would love to see sidekiq-cron move back to stability.

"sudden loss of compatibility"... Please read http://jmettraux.skepti.ch/20180601.html?t=Gemfile_lock&f=200

Regarding !errors.any? and the typo fix, granted, it's unrelated.

Regarding sidekiq-cron.gemspec, please read https://github.com/ondrejbartas/sidekiq-cron/blob/master/sidekiq-cron.gemspec#L1-L5

I respect that you feel strongly about your chosen styles

That leaves just !errors.any? vs errors.empty? as part of my chosen styles.

I trust @ondrejbartas will do the right thing when he has time. I don't care if he rejects my PR, it's rather a message from me to him, showing how to go from rufus-scheduler to fugit. I have interacted with him before and I must say he's a nice chap. I'm sure he'll do what he, as project owner, feels is right and I will respect his choices.

Kind regards.

@Supernats

This comment has been minimized.

Supernats commented Jun 7, 2018

I didn't see that line about using Jeweler. Thanks for pointing that out to me.

I believe I misspoke with the my use of the word "sudden," and for that I apologize. In hindsight, a better word would have been "unexpected," or perhaps no descriptor at all. We check in our Gemfile.lock, as you are absolutely correct in advising others do, and this never caused us a moment of downtime.

Thanks for your contributions and taking the time to courteously address someone looking to help this gem move back toward stability for the community.

@fschwahn

This comment has been minimized.

fschwahn commented Jun 21, 2018

The test failures are a result of a weird issue I think, namely that tests fail when run after 11pm because of

enqueued_time = Time.new(now.year, now.month, now.day, now.hour + 1, 10, 5)

If hour is 23, it will create an invalid time.

What's interesting though is that (when run on travis) one of the performance tests fails in all ruby versions. Is it possible that the update to fugit could cause such a performance regression? I looked at the history of test failures of the repo, and couldn't find another instance of it failing.

@jmettraux

This comment has been minimized.

Contributor

jmettraux commented Jun 22, 2018

@fschwahn wrote:

Is it possible that the update to fugit could cause such a performance regression?

I'm looking into it. Thanks for the initial analysis!

@jmettraux

This comment has been minimized.

Contributor

jmettraux commented Jun 22, 2018

Caching the parsed cron instance solved the performance regression.

Green.

Thanks again @fschwahn

@ondrejbartas ondrejbartas merged commit ab362b3 into ondrejbartas:master Jul 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jmettraux jmettraux deleted the jmettraux:fugit_1.1 branch Jul 9, 2018

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