floraison / fugit

time tools (cron, parsing, durations, ...) for Ruby, rufus-scheduler, and flor
MIT License
355 stars 29 forks source link

Adds timezone and duration support for Fugit:Nat #7

Closed cristianbica closed 5 years ago

jmettraux commented 5 years ago

Many thanks!

jmettraux commented 5 years ago

@cristianbica Thanks again for your contribution.

Although, I somehow regret including the "duration" aspect of Fugit::Nat.

In the context of rufus-scheduler from where fugit comes, "every 5 minutes" is "programmed" as scheduler.every('5m') { do_something() } not as scheduler.cron('*/5 * * * *').

I am wondering if fugit Fugit::Nat.parse('every 5 minutes') shouldn't yield 300 (seconds) instead of a Fugit::Cron instance, unless explicitely forced into a cron, Fugit::Cron.parse('every 5 minutes') or Fugit::Nat.parse('every 5 minutes', cron: true) or Fugit.parse(('every 5 minutes', cron: true).

I'm thinking about wiring this "natural" parsing into rufus-scheduler 3.5.2, so for me complying with rufus-scheduler is important.

What is your context? Are you using fugit standalone? What are your needs?

jmettraux commented 5 years ago

I am also tempted to let Fugit::Nat returns an array of results instead of a single result.

Fugit::Nat.parse('every day at five thirty and six twenty')
  # ==> [ <Fugit::Cron "30 5 * * *">, <Fugit::Cron "20 6 * * *"> ]
cristianbica commented 5 years ago

Hey @jmettraux

I'm working at a simple gem I need to schedule recurring ActiveJobs. The idea of the gem is to be a wrapper for rufus and for each job I'd declare the recurrence and at runtime a separate rufus process needs to be run.

The recurrence declaration I want to support is:

class TestJob < ApplicationJob
  repeat "every 2 hours",
         "0 4 * * * America/New_York",
         "every Monday at 4 am on America/Los_Angeles"
end

I'm not too sure about the outcome of this PR. There's a subtle difference between every 5 minutes and */5 * * * * which is not straight forward to users. On the other hand Fugit::Nat.parse('every 5 minutes') yielding 300 it's not very "natural" too me because of the every word.

If it were up to me I wouldn't support multiple recurrences in the same call. This adds a level of complexity to the whole thing which could by easily be solved by users scheduling the same job twice.

I'm also a but worried about rufus knowing too much ( :) ). Given that you're extracting the recurrence logic into fugit then rufus shouldn't be aware of cron or natural syntax. Fugit should return a Fugit::Recurrence object which will handle the recurrence logic for rufus. Rufus should provide only a base public API .schedule(some_recurrence_string). All the other methods are just syntax sugar (.in("5 minutes") is actually schedule("in 5 minutes"). It will be challenging encapsulating the recurrence logic into Fugit::Recurrence but cron parsing and natural language parsing are a bit messy and should be contained.

jmettraux commented 5 years ago

Hello @cristianbica,

There's a subtle difference between every 5 minutes and /5 * which is not straight forward to users.

Indeed.

If it were up to me I wouldn't support multiple recurrences in the same call. This adds a level of complexity to the whole thing which could by easily be solved by users scheduling the same job twice.

Very good point.

Fugit should return a Fugit::Recurrence object which will handle the recurrence logic for rufus.

That's a good idea. I'll think about it.

Thanks for explaining your context and the excellent pieces of advice. I have to spend some time pondering over them.

Kind regards.