Skip to content
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

Sub second precision is ignored and .next returns time in past for same second #160

Open
doits opened this issue Sep 15, 2023 · 1 comment

Comments

@doits
Copy link

doits commented Sep 15, 2023

According to this line in the specs ...

assert frequency.include? now
... it seems to be intended, but just now it triggered a bug for me:

Montrose.every(:second).events.next de facto returns a time in past, because it will always return a time that has sub second time set to 0.

For example if you have a daily trigger Montrose.every(:day).at('18:00:00') and you run events.next at 15.01.2020 18:00:00.500 (half a second past 18:00:00), it returns 15.01.2020 18:00:00.000 which is in past (at least if you care about sub second precision).

Is this a bug? Shouldn't events.next never return a time in the past?

Edit: A workaround I just implemented is to do Montrose::Recurrence.new(recurrence).take(3).find { |time| time >= Time.current }.

@rossta
Copy link
Owner

rossta commented Oct 12, 2023

Montrose doesn't support sub-second recurrence. When a recurrence is initialized, the start time for the recurrence interval is zero'd out to the current second.

The issue you are describing will only exist when a recurrence is initialized at less than a second after a time which overlaps with the recurrence rules, as you have illustrated.

Is this a bug?

Probably, though I'd consider it minor. A simple patch with tests to address the issue is welcome.

Shouldn't events.next never return a time in the past?

We do generally want to support events.next returning time in the past (since the start timestamp may be explicitly set in the past) though I understand what you mean in the context of this issue.

A workaround...

Your workaround may lead to subtle bugs as well, since Time.current is evaluated anew each time your find block is run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants