floraison / fugit

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

Un-representable inputs should raise an error #86

Closed DanielHeath closed 8 months ago

DanielHeath commented 10 months ago

Issue description

Fugit.parse will happily accept every 17 hours as a time specification, but cannot represent that interval in a crontab.

How to reproduce

The simplest piece of code that reproduces the issue, for example:

[17] pry(main)> Fugit.parse('every 17 hours').brute_frequency
=> #<Fugit::Cron::Frequency:0x00007f9c8cafd900
 @delta_max=61200,
 @delta_min=25200,
 @occurrences=730,
 @span=31536000.0,
 @span_years=1.0,
 @yearly_occurrences=730.0>

[18] pry(main)> Fugit.parse('every 27 hours').brute_frequency
=> #<Fugit::Cron::Frequency:0x00007f9c8c03b238
 @delta_max=86400,
 @delta_min=86400,
 @occurrences=365,
 @span=31536000.0,
 @span_years=1.0,
 @yearly_occurrences=365.0>

Expected behavior

ArgumentError would be an appropriate exception to raise if an argument can't be processed correctly

Context

Please replace the content of this section with the output of the following commands:

Linux drh-workstation 5.15.0-79-generic #86-Ubuntu SMP Mon Jul 10 16:07:21 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
ruby 3.2.2 (2023-03-30 revision e51014f9c0) [x86_64-linux]
[:env_tz, nil]
(secs:1692925595.6963181,utc~:"2023-08-25 01:06:35.6963181495666504",ltz~:"AEST")
(etz:nil,tnz:"AEST",tziv:"2.0.6",tzidv:nil,rv:"3.2.2",rp:"x86_64-linux",win:false,rorv:nil,astz:nil,eov:"1.2.7",eotnz:#<TZInfo::DataTimezone: Australia/Melbourne>,eotnfz:"+1000",eotlzn:"Australia/Melbourne",eotnfZ:"AEST",debian:"Australia/Melbourne",centos:nil,osx:"Australia/Melbourne")
[:fugit, "1.8.1"]
[:now, 2023-08-25 11:06:36.581398905 +1000, :zone, "AEST"]
jmettraux commented 10 months ago

Hello,

thanks for this report.

Fugit.parse is not supposed to raise an ArgumentError on invalid input, it simply returns nil. Fugit.do_parse will raise on invalid input.

The fugit nat "every" constructs produces Fugit::Cron instances. For example "every 3 hours" is turned into "0 /3 ". It breaks quickly as you noticed, with "every 17 hours" turned into "0 /17 " which, in fact, corresponds to "every day at midnight and at 5 pm".

I am OK with returning nil / raising an ArgumentError for "every 27 hours", but for "every 17 hours", I lean towards returning nil for Fugit.parse("every 17 hours", strict: true), as seen in https://github.com/floraison/fugit/commit/ec91e88ab4ca4496b208dc6d62cebbc9c2bacaa5

Would that suit you?

Kind regards.

DanielHeath commented 10 months ago

strict: true would do it, yeah.

jmettraux commented 8 months ago

You're welcome.

DanielHeath commented 8 months ago

:tada: Thank you!