Codecov Report

Patch coverage: 100.00% and project coverage change: -0.34 ⚠️

Comparison is base (e9bcc5a) 99.08% compared to head (46e47f4) 98.74%.

❗ Current head 46e47f4 differs from pull request most recent head 087e544. Consider uploading reports for the commit 087e544 to get more accurate results

@@            Coverage Diff             @@
##           master      #25      +/-   ##
==========================================
- Coverage   99.08%   98.74%   -0.34%     
==========================================
  Files          83       83              
  Lines        2069     2149      +80     
==========================================
+ Hits         2050     2122      +72     
- Misses         19       27       +8     

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

I wonder if it makes sense to validate separate keys for periodical? subclass Drip? Spitballing here, not a prescription.

I could totally see that. Actually the separation between drip and periodical totally isn't clear right now and may well be a foot-gun as is since you can pass every: (and now until:) and start: to drip and it will work for the first mailing, but the subsequent mailings won't get re-enqueued! So you have to use periodical, but I think that's both not-currently documented and a super-hazard that's easy to do. We were about to use drip .. every: 2.weeks before realizing the subsequent-repeating wouldn't have fired if we did.

I'll push it up in a sec; might need some help documenting it... hehe

can you help with #26? i think once that's merged this might be more tenable?

Yeah!

one thing about this:

keyword arg until will throw a fit cuz it's reserved. :)

perhaps if?

perhaps if?

We'd have the same problem. The keyword issue is that we're passing the arg into a hash and it would look like this:

        def periodical(action_name, every:, start: -> { ::Caffeinate.config.time_now }, until: -> { 5_000.years.from_now }**options, &block)
           options[:start] = start
           options[:every] = every
           options[:until] = until

           drip(action_name, options, &block)

           # etc.

Even Github's Ruby parser marks the until word there ^ red 😆 can't use until as an argument name. if would have the same problem. So I just didn't break it out from options and set it with ||= instead:

         def periodical(action_name, every:, start: -> { ::Caffeinate.config.time_now }, **options, &block)
           options[:start] = start
           options[:every] = every
           options[:until] ||= 5_000.years.from_now # can't call this out as a param above because `until` is a Ruby keyword

           drip(action_name, options, &block)

           # etc.

But outside of Ruby language logistics (and the trouble that comes with either until or if) the question is, which term presents a more readily-understandable and simple interface?

"Do this every X days, starting Y, until Z"

or

"Do this every X days, starting Y, if Z"

"Do this every X days, starting Y, until Z"

or

"Do this every X days, starting Y, if Z"

I think between these two I'm still on team until 🤔

(perhaps if was a really bad attempt at a joke)

do we want to merge #26 first?

(perhaps if was a really bad attempt at a joke)

🤣 then lol at my totally not getting that and further lol at my explanation

Yeah, no rush here; I'm in favor of getting #26 cleared beforehand. I should have time tomorrow to look through #24 and #26 more fully!

@joshmn any interest in this PR still? Happy to rebase; I think master still has a couple of hiccups / bugs around start only being used in the first iteration of a periodical — hope to iron that out even if we don't end up going with :until

Note to self: I should come back to this, and maybe instead of having until: <proc> we use while: <PROC> key, since "while" naturally implies a "while this is true" API, which makes simpler cognitive sense than "until this is true? until this is false?"

I don't think this PR is needed anymore. See extensive writeup at #41