Skip to content

Describe explicit minute 0 when seconds are not implicit - #717

Open
austek wants to merge 1 commit into
jmrozanec:masterfrom
austek:fix/issue_3
Open

Describe explicit minute 0 when seconds are not implicit#717
austek wants to merge 1 commit into
jmrozanec:masterfrom
austek:fix/issue_3

Conversation

@austek

@austek austek commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Description of your changes

TimeDescriptionStrategy always suppressed the minute when it was On(0), treating minute 0 as an implied default. That reads well when the seconds are implicit too, but when they are not, the description asserts a sub-minute frequency with nothing bounding it to the minute the cron actually fires on:

* 0 9-23 * * ?   before: every second every hour between 9 and 23
                 after:  every second at 0 minute every hour between 9 and 23

This is the case reported in #3 by @natrajmolala, where you invited a PR with a test.

The fix gates the existing suppression on the seconds also being default. Three related expressions were wrong in the same way and are corrected by the same condition:

expression before after
5 0 9-23 * * ? at 5 second every hour between 9 and 23 at 5 second at 0 minute every hour between 9 and 23
0/5 0 * * * ? every 5 seconds from second 0 every 5 seconds from second 0 at 0 minute
* 0 */4 * * ? every second every 4 hours every second at 0 minute every 4 hours

Where the suppression is deliberate it is preserved — 0 0 9-23 * * ? still describes as "every hour between 9 and 23" and 0 0 10 * * ? as "at 10:00". Issue3Test pins those too, so the collapse cannot regress.

Scope

#3 is an umbrella issue and this closes one reported case out of four; the others are phrasing rather than correctness, so the commit says Refs #3 rather than Fixes.

Incidental

The strategy set is now insertion ordered and the duplicated predicate call in the match loop is hoisted. Both are hardening only — the eleven registered predicates cover mutually exclusive (hours, minutes, seconds) type triples, so the previous HashSet iteration order was not reachable as nondeterministic output.

Impact Analysis

Output changes only for expressions that fire on an explicit minute 0 with non-default seconds, which were describing roughly sixty times the firings they actually produce. Callers asserting on those exact strings will need updating. No API, parser or i18n bundle changes, so no new resource keys and no impact on the non-English locales.

Checklist

  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • Existing tests pass: 810 tests, 0 failures, 0 errors (up from 802 on master by the 8 added cases; 57 pre-existing skips)

Minute 0 was always suppressed as an implied default, so an expression
firing on a sub-minute schedule lost the minute bounding it. "* 0 9-23 * * ?"
described as "every second every hour between 9 and 23", roughly sixty times
the firings it actually produces.

Only suppress minute 0 when the seconds are implicit too. Also fixes
"5 0 9-23 * * ?", "0/5 0 * * * ?" and "* 0 */4 * * ?", and covers the
deliberate suppression cases so they stay collapsed.

Makes the strategy set insertion ordered and hoists the duplicated
predicate call while here.

Refs jmrozanec#3
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

Successfully merging this pull request may close these issues.

1 participant