fix(cron): stop describing a month-restricted schedule as daily - #319
fix(cron): stop describing a month-restricted schedule as daily#319naufalfx805-source wants to merge 1 commit into
Conversation
describeCron destructured the 5 cron fields but skipped the month, so a
schedule restricted to certain months was described by its finer fields
alone. `0 3 * 6 *` -- daily at 03:00, but only in June -- came back as
"daily at 03:00".
That description is printed by formatScheduleFrequencyAdvisory beside a
frequency that runsPerMonth computes correctly, so `schedule create` and
`schedule update` rendered a sentence contradicting itself:
This schedule will run ~3 time(s)/month (daily at 03:00).
None of the phrasings describeCron produces can carry "only in these
months", so a restricted month now falls back to the raw expression --
the same thing the function already does for every other shape it cannot
state exactly, and what its own docstring asks for: "a wrong description
is worse than the raw expression".
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. Walkthrough
ChangesCron description fix
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Month-restricted schedules now retain their raw cron expression instead of being described as daily, preventing contradictory schedule advisories. The change is localized and tested, with no actionable merge-blocking risk remaining beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
Closes #318
describeCrondestructured the five cron fields but skipped the month, so a schedule restricted to certain months was described by its finer fields alone.0 3 * 6 *— daily at 03:00, but only in June — came back asdaily at 03:00.That string is printed by
formatScheduleFrequencyAdvisorybeside a frequencyrunsPerMonthcomputes correctly, soschedule create(src/commands/schedule.ts:245) andschedule update(:347) rendered a sentence that contradicts itself:Fix. None of the phrasings
describeCronproduces can carry "only in these months", so a restricted month now falls back to the raw expression — the same thing the function already does for every other shape it cannot state exactly, and what its own docstring asks for: "a wrong description is worse than the raw expression".runsPerMonthis untouched; it already handled the month correctly.Tests. Two added, both verified failing on
mainand passing with the change:describeCron > returns the raw expression when the month is restricted— covers0 3 * 6 *,0 3 * 1,7 *,0 3 * */6 *,0 3 1 3 *,0 3 * 6 1. Without the fix:expected 'daily at 03:00' to be '0 3 * 6 *'.formatScheduleFrequencyAdvisory > does not call a month-restricted schedule daily— asserts the advisory keeps~3 time(s)/monthand stops sayingdaily.npx vitest run src/lib/cron.test.ts→ 24 passed.src/commands/schedule→ 66 passed.The gap in the existing suite was that
describeCronwas never exercised with a non-wildcard month, and the one advisory test that uses one (0 3 1 1 *) asserts only the frequency half, not the description.Summary by CodeRabbit