From 68dea047601ecdb72d81c0858fd0b578ae336d5d Mon Sep 17 00:00:00 2001 From: austek <13117393+austek@users.noreply.github.com> Date: Tue, 28 Jul 2026 15:28:05 +0100 Subject: [PATCH 1/4] Name the position of an nth day of week "0 59 10 ? 1/2 MON#1 *" read "at 10:59 every 2 months from month 1 Monday 1 of every month". The nth day of week printed its position as a bare number, and claimed "of every month" right after the month field had restricted it to every second month. Give the # and L forms their own phrasing, so they read "on the first Monday of the month" and "on the last Friday of the month" and no longer contradict a restricted month. Positions past the fifth, outside the Quartz range but reachable through the generic descriptor, fall back to the plain number. Field order is untouched, so the day of week still follows the month rather than leading the sentence as the report suggests. Fixes #126 --- .../DescriptionStrategyFactory.java | 14 ++++- .../com/cronutils/CronUtilsI18N.properties | 7 +++ .../utils/descriptor/CronDescriptorTest.java | 4 +- .../utils/descriptor/Issue126Test.java | 56 +++++++++++++++++++ 4 files changed, 77 insertions(+), 4 deletions(-) create mode 100644 src/test/java/com/cronutils/utils/descriptor/Issue126Test.java diff --git a/src/main/java/com/cronutils/descriptor/DescriptionStrategyFactory.java b/src/main/java/com/cronutils/descriptor/DescriptionStrategyFactory.java index 61920609..3b31d7d8 100755 --- a/src/main/java/com/cronutils/descriptor/DescriptionStrategyFactory.java +++ b/src/main/java/com/cronutils/descriptor/DescriptionStrategyFactory.java @@ -31,6 +31,15 @@ class DescriptionStrategyFactory { private DescriptionStrategyFactory() { } + /** + * Names the position an nth day of week occupies, falling back to the plain number for + * positions no bundle spells out. + */ + private static String ordinal(final int nth, final ResourceBundle bundle) { + final String key = "nth_" + nth; + return bundle.containsKey(key) ? bundle.getString(key) : String.valueOf(nth); + } + /** * Creates description strategy for days of week. * @@ -54,9 +63,10 @@ public static DescriptionStrategy daysOfWeekInstance(final ResourceBundle bundle final On on = (On) fieldExpression; switch (on.getSpecialChar().getValue()) { case HASH: - return String.format("%s %s %s ", nominal.apply(on.getTime().getValue()), on.getNth(), bundle.getString("of_every_month")); + return MessageFormat.format(bundle.getString("on_nth_day_of_week_x"), + ordinal(on.getNth().getValue(), bundle), nominal.apply(on.getTime().getValue())); case L: - return String.format("%s %s %s ", bundle.getString("last"), nominal.apply(on.getTime().getValue()), bundle.getString("of_every_month")); + return MessageFormat.format(bundle.getString("on_last_day_of_week_x"), nominal.apply(on.getTime().getValue())); default: return ""; } diff --git a/src/main/resources/com/cronutils/CronUtilsI18N.properties b/src/main/resources/com/cronutils/CronUtilsI18N.properties index 027f0089..9c349351 100644 --- a/src/main/resources/com/cronutils/CronUtilsI18N.properties +++ b/src/main/resources/com/cronutils/CronUtilsI18N.properties @@ -16,6 +16,13 @@ months=months year=year years=years between=between +on_nth_day_of_week_x=on the {0} {1} of the month +on_last_day_of_week_x=on the last {0} of the month +nth_1=first +nth_2=second +nth_3=third +nth_4=fourth +nth_5=fifth of_every_month=of every month of_the_month=of the month last=last diff --git a/src/test/java/com/cronutils/utils/descriptor/CronDescriptorTest.java b/src/test/java/com/cronutils/utils/descriptor/CronDescriptorTest.java index 4cc60f79..571a6b79 100755 --- a/src/test/java/com/cronutils/utils/descriptor/CronDescriptorTest.java +++ b/src/test/java/com/cronutils/utils/descriptor/CronDescriptorTest.java @@ -148,7 +148,7 @@ public void testLastDayOfWeekInMonth() { results.add(new CronField(CronFieldName.MINUTE, new On(new IntegerFieldValue(minute)), nullFieldConstraints)); results.add(new CronField(CronFieldName.DAY_OF_WEEK, new On(new IntegerFieldValue(dayOfWeek), new SpecialCharFieldValue(SpecialChar.L)), nullFieldConstraints)); - assertEquals(String.format("at %s:%s last Tuesday of every month", hour, minute), descriptor.describe(new SingleCron(mockDefinition, results))); + assertEquals(String.format("at %s:%s on the last Tuesday of the month", hour, minute), descriptor.describe(new SingleCron(mockDefinition, results))); } @Test @@ -161,7 +161,7 @@ public void testNthDayOfWeekInMonth() { results.add(new CronField(CronFieldName.MINUTE, new On(new IntegerFieldValue(minute)), nullFieldConstraints)); results.add(new CronField(CronFieldName.DAY_OF_WEEK, new On(new IntegerFieldValue(dayOfWeek), new SpecialCharFieldValue(SpecialChar.HASH), new IntegerFieldValue(dayOfWeek)), nullFieldConstraints)); - assertEquals(String.format("at %s:%s Tuesday %s of every month", hour, minute, dayOfWeek), descriptor.describe(new SingleCron(mockDefinition, results))); + assertEquals(String.format("at %s:%s on the second Tuesday of the month", hour, minute), descriptor.describe(new SingleCron(mockDefinition, results))); } @Test diff --git a/src/test/java/com/cronutils/utils/descriptor/Issue126Test.java b/src/test/java/com/cronutils/utils/descriptor/Issue126Test.java new file mode 100644 index 00000000..6c9aea3c --- /dev/null +++ b/src/test/java/com/cronutils/utils/descriptor/Issue126Test.java @@ -0,0 +1,56 @@ +package com.cronutils.utils.descriptor; + +import com.cronutils.descriptor.CronDescriptor; +import com.cronutils.model.CronType; +import com.cronutils.model.definition.CronDefinitionBuilder; +import com.cronutils.parser.CronParser; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; + +import java.util.Locale; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** + * Issue 126 - "0 59 10 ? 1/2 MON#1 *" read as "at 10:59 every February months Monday 1 of every + * month". The nth day of week was spelled out as a bare number and claimed to happen "of every + * month", contradicting the month field that had just restricted it. + */ +public class Issue126Test { + + private final CronParser parser = new CronParser(CronDefinitionBuilder.instanceDefinitionFor(CronType.QUARTZ)); + + @ParameterizedTest + @CsvSource({ + "'0 59 10 ? * MON#1 *', 'at 10:59 on the first Monday of the month'", + "'0 0 0 ? * TUE#2', 'at 00:00 on the second Tuesday of the month'", + "'0 0 0 ? * WED#3', 'at 00:00 on the third Wednesday of the month'", + "'0 0 0 ? * THU#4', 'at 00:00 on the fourth Thursday of the month'", + "'0 0 0 ? * FRI#5', 'at 00:00 on the fifth Friday of the month'" + }) + public void nthDayOfWeekNamesItsPosition(String expression, String expected) { + assertEquals(expected, CronDescriptor.instance(Locale.ENGLISH).describe(parser.parse(expression))); + } + + @ParameterizedTest + @CsvSource({ + "'0 0 0 ? * MONL', 'at 00:00 on the last Monday of the month'", + "'0 0 0 ? * 6L', 'at 00:00 on the last Friday of the month'" + }) + public void lastDayOfWeekReadsTheSameWay(String expression, String expected) { + assertEquals(expected, CronDescriptor.instance(Locale.ENGLISH).describe(parser.parse(expression))); + } + + /** + * The expression from the report: a restricted month must not be followed by a day of week + * claiming every month. + */ + @ParameterizedTest + @CsvSource({ + "'0 59 10 ? 1/2 MON#1 *', 'at 10:59 every 2 months from month 1 on the first Monday of the month'", + "'0 59 10 ? 3 MON#1 *', 'at 10:59 at March month on the first Monday of the month'" + }) + public void aRestrictedMonthIsNotContradicted(String expression, String expected) { + assertEquals(expected, CronDescriptor.instance(Locale.ENGLISH).describe(parser.parse(expression))); + } +} From e5baf91ab9de8f383c1566488caf15604d267a45 Mon Sep 17 00:00:00 2001 From: austek <13117393+austek@users.noreply.github.com> Date: Tue, 28 Jul 2026 13:25:41 +0100 Subject: [PATCH 2/4] Describe explicit minute 0 when seconds are not implicit 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 #3 --- .../descriptor/TimeDescriptionStrategy.java | 15 +++--- .../utils/descriptor/Issue3Test.java | 51 +++++++++++++++++++ 2 files changed, 60 insertions(+), 6 deletions(-) create mode 100644 src/test/java/com/cronutils/utils/descriptor/Issue3Test.java diff --git a/src/main/java/com/cronutils/descriptor/TimeDescriptionStrategy.java b/src/main/java/com/cronutils/descriptor/TimeDescriptionStrategy.java index bd397840..1d3f8634 100644 --- a/src/main/java/com/cronutils/descriptor/TimeDescriptionStrategy.java +++ b/src/main/java/com/cronutils/descriptor/TimeDescriptionStrategy.java @@ -19,7 +19,7 @@ import com.cronutils.utils.Preconditions; import com.cronutils.utils.StringUtils; -import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.ResourceBundle; import java.util.Set; @@ -59,7 +59,7 @@ class TimeDescriptionStrategy extends DescriptionStrategy { this.hours = ensureInstance(hours, always()); this.minutes = ensureInstance(minutes, always()); this.seconds = ensureInstance(seconds, new On(new IntegerFieldValue(DEFAULTSECONDS))); - descriptions = new HashSet<>(); + descriptions = new LinkedHashSet<>(); registerFunctions(); } @@ -85,20 +85,23 @@ private FieldExpression ensureInstance(final FieldExpression expression, final F public String describe() { final TimeFields fields = new TimeFields(hours, minutes, seconds); for (final Function function : descriptions) { - if (!"".equals(function.apply(fields))) { - return function.apply(fields); + final String description = function.apply(fields); + if (!"".equals(description)) { + return description; } } String secondsDesc = ""; String minutesDesc = ""; String hoursDesc = ""; + // minute 0 may only stay implicit while seconds are too, else we claim an unbounded sub-minute frequency + final boolean defaultSeconds = seconds instanceof On && isDefault((On) seconds); if (!(hours instanceof Always)) { hoursDesc = addTimeExpressions(describe(hours), bundle.getString(HOUR), bundle.getString("hours")); } - if (!(minutes instanceof On && isDefault((On) minutes)) && !((minutes instanceof Always) && (hours instanceof Always))) { + if (!(minutes instanceof On && isDefault((On) minutes) && defaultSeconds) && !((minutes instanceof Always) && (hours instanceof Always))) { minutesDesc = addTimeExpressions(describe(minutes), bundle.getString(MINUTE), bundle.getString("minutes")); } - if (!(seconds instanceof On && isDefault((On) seconds))) { + if (!defaultSeconds) { secondsDesc = addTimeExpressions(describe(seconds), bundle.getString(SECOND), bundle.getString("seconds")); } return String.format("%s %s %s", secondsDesc, minutesDesc, hoursDesc); diff --git a/src/test/java/com/cronutils/utils/descriptor/Issue3Test.java b/src/test/java/com/cronutils/utils/descriptor/Issue3Test.java new file mode 100644 index 00000000..b96de220 --- /dev/null +++ b/src/test/java/com/cronutils/utils/descriptor/Issue3Test.java @@ -0,0 +1,51 @@ +package com.cronutils.utils.descriptor; + +import com.cronutils.descriptor.CronDescriptor; +import com.cronutils.model.CronType; +import com.cronutils.model.definition.CronDefinitionBuilder; +import com.cronutils.parser.CronParser; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; + +import java.util.Locale; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** + * Issue 3 - a cron firing on an explicit minute 0 lost that minute in its description, so + * "* 0 9-23 * * ?" read as "every second every hour between 9 and 23" and claimed roughly + * sixty times the firings the expression actually produces. + */ +public class Issue3Test { + + private final CronParser parser = new CronParser(CronDefinitionBuilder.instanceDefinitionFor(CronType.QUARTZ)); + + @ParameterizedTest + @CsvSource({ + "'* 0 9-23 * * ?', 'every second at 0 minute every hour between 9 and 23'", + "'5 0 9-23 * * ?', 'at 5 second at 0 minute every hour between 9 and 23'", + "'0/5 0 * * * ?', 'every 5 seconds from second 0 at 0 minute'", + "'* 0 */4 * * ?', 'every second at 0 minute every 4 hours'" + }) + public void explicitMinuteZeroIsDescribedWhenSecondsAreNotImplicit(String expression, String expected) { + assertEquals(expected, CronDescriptor.instance(Locale.ENGLISH).describe(parser.parse(expression))); + } + + @ParameterizedTest + @CsvSource({ + "'0 0 9-23 * * ?', 'every hour between 9 and 23'", + "'0 0 10 * * ?', 'at 10:00'" + }) + public void minuteZeroStaysImplicitWhenSecondsAreImplicitToo(String expression, String expected) { + assertEquals(expected, CronDescriptor.instance(Locale.ENGLISH).describe(parser.parse(expression))); + } + + @ParameterizedTest + @CsvSource({ + "'* 30 9-23 * * ?', 'every second at 30 minute every hour between 9 and 23'", + "'* 0 10 * * ?', 'every second at 10:00'" + }) + public void nonZeroAndCollapsibleMinutesAreUnaffected(String expression, String expected) { + assertEquals(expected, CronDescriptor.instance(Locale.ENGLISH).describe(parser.parse(expression))); + } +} From 746b092607f7e075a126dd801a71d7cc9042ddbd Mon Sep 17 00:00:00 2001 From: austek <13117393+austek@users.noreply.github.com> Date: Tue, 28 Jul 2026 13:57:30 +0100 Subject: [PATCH 3/4] Read day of month, month and year as prose "0 0 0 1 1 ?" described as "at 00:00 at 1 day at January month" and "0 15 10 * * ? 2005" as "at 10:15 at 2005 year". Give day of month, month and year their own phrasing so they read "on day 1 in January" and "in 2005". Drop the start of an every expression when the period is 1 and the start is 0, since "every 1 unit from 0" is just "every unit", so "0/1 * * * * ?" is now "every second" rather than "every second from second 0". A meaningful start is kept: "5/1 * * * * ?" still reads "every second from second 5". Omit the unit word where the value already names it, so months, years and days of week read "from October" rather than "from month 10". Months no longer fall back to the raw number inside an every expression, which is what produced "month 10". Months and years take separate bundle keys even though English formats both as "in {0}", so a translator can decline them differently. The keys are only in the base bundle for now, so other locales fall back to English for these phrases. Refs #3 --- .../cronutils/descriptor/CronDescriptor.java | 2 +- .../descriptor/DescriptionStrategy.java | 8 +++- .../DescriptionStrategyFactory.java | 44 +++++++++++++++---- .../NominalDescriptionStrategy.java | 14 +++++- .../com/cronutils/CronUtilsI18N.properties | 3 ++ .../CronDescriptorQuartzIntegrationTest.java | 6 +-- .../utils/descriptor/CronDescriptorTest.java | 2 +- .../utils/descriptor/Issue126Test.java | 4 +- .../utils/descriptor/Issue3Test.java | 41 +++++++++++++++++ 9 files changed, 106 insertions(+), 18 deletions(-) diff --git a/src/main/java/com/cronutils/descriptor/CronDescriptor.java b/src/main/java/com/cronutils/descriptor/CronDescriptor.java index fe5e2468..834a7ccd 100755 --- a/src/main/java/com/cronutils/descriptor/CronDescriptor.java +++ b/src/main/java/com/cronutils/descriptor/CronDescriptor.java @@ -162,7 +162,7 @@ private String addExpressions(final String description, final String singular, f */ public String describeYear(final Map fields) { final String description = - DescriptionStrategyFactory.plainInstance( + DescriptionStrategyFactory.yearsInstance( resourceBundle, fields.containsKey(CronFieldName.YEAR) ? fields.get(CronFieldName.YEAR).getExpression() : null ).describe(); diff --git a/src/main/java/com/cronutils/descriptor/DescriptionStrategy.java b/src/main/java/com/cronutils/descriptor/DescriptionStrategy.java index 1762c14e..d9c0aafd 100755 --- a/src/main/java/com/cronutils/descriptor/DescriptionStrategy.java +++ b/src/main/java/com/cronutils/descriptor/DescriptionStrategy.java @@ -34,6 +34,8 @@ abstract class DescriptionStrategy { private static final String WHITE_SPACE = " "; protected Function nominalValueFunction; protected ResourceBundle bundle; + // true when a value already names its own unit ("October", "Tuesday"), so the unit word is redundant + protected boolean selfDescribingValues; public DescriptionStrategy(final ResourceBundle bundle) { this.bundle = bundle; @@ -169,7 +171,11 @@ protected String describe(final Every every, final boolean and) { } if (every.getExpression() instanceof On) { final On on = (On) every.getExpression(); - description += bundle.getString("from") + " %s " + nominalValue(on.getTime()); + // "every 1 unit starting at 0" is just "every unit"; 0 is out of range for the 1-based fields + if (every.getPeriod().getValue() == 1 && on.getTime().getValue() == 0) { + return description; + } + description += bundle.getString("from") + (selfDescribingValues ? WHITE_SPACE : " %s ") + nominalValue(on.getTime()); } return description; } diff --git a/src/main/java/com/cronutils/descriptor/DescriptionStrategyFactory.java b/src/main/java/com/cronutils/descriptor/DescriptionStrategyFactory.java index 3b31d7d8..942fdbcd 100755 --- a/src/main/java/com/cronutils/descriptor/DescriptionStrategyFactory.java +++ b/src/main/java/com/cronutils/descriptor/DescriptionStrategyFactory.java @@ -19,6 +19,7 @@ import com.cronutils.model.field.expression.Every; import com.cronutils.model.field.expression.FieldExpression; import com.cronutils.model.field.expression.On; +import com.cronutils.model.field.value.SpecialChar; import java.text.MessageFormat; import java.time.DayOfWeek; @@ -56,7 +57,7 @@ public static DescriptionStrategy daysOfWeekInstance(final ResourceBundle bundle return DayOfWeek.of(integer + diff < 1 ? 7 : integer + diff).getDisplayName(TextStyle.FULL, bundle.getLocale()); }; - final NominalDescriptionStrategy dow = new NominalDescriptionStrategy(bundle, nominal, expression); + final NominalDescriptionStrategy dow = new NominalDescriptionStrategy(bundle, nominal, expression).withSelfDescribingValues(); dow.addDescription(fieldExpression -> { if (fieldExpression instanceof On) { @@ -104,6 +105,8 @@ public static DescriptionStrategy daysOfMonthInstance(final ResourceBundle bundl } case LW: return bundle.getString("last_weekday_of_month"); + case NONE: + return MessageFormat.format(bundle.getString("on_day_x"), on.getTime().getValue()); default: return ""; } @@ -121,13 +124,38 @@ public static DescriptionStrategy daysOfMonthInstance(final ResourceBundle bundl * @return - DescriptionStrategy instance, never null */ public static DescriptionStrategy monthsInstance(final ResourceBundle bundle, final FieldExpression expression) { - Function mappingFunction; - if (expression instanceof Every) { - mappingFunction = Object::toString; - } else { - mappingFunction = integer -> Month.of(integer).getDisplayName(TextStyle.FULL, bundle.getLocale()); - } - return new NominalDescriptionStrategy(bundle, mappingFunction, expression); + final Function nominal = integer -> Month.of(integer).getDisplayName(TextStyle.FULL, bundle.getLocale()); + final NominalDescriptionStrategy months = + new NominalDescriptionStrategy(bundle, nominal, expression).withSelfDescribingValues(); + + months.addDescription(fieldExpression -> { + if (fieldExpression instanceof On && ((On) fieldExpression).getSpecialChar().getValue() == SpecialChar.NONE) { + return MessageFormat.format(bundle.getString("in_month_x"), nominal.apply(((On) fieldExpression).getTime().getValue())); + } + return ""; + }); + return months; + } + + /** + * Creates description strategy for years. + * + * @param bundle - locale + * @param expression - CronFieldExpression + * @return - DescriptionStrategy instance, never null + */ + public static DescriptionStrategy yearsInstance(final ResourceBundle bundle, final FieldExpression expression) { + final NominalDescriptionStrategy years = + new NominalDescriptionStrategy(bundle, null, expression).withSelfDescribingValues(); + + years.addDescription(fieldExpression -> { + if (fieldExpression instanceof On && ((On) fieldExpression).getSpecialChar().getValue() == SpecialChar.NONE) { + // as a String, so MessageFormat does not group four digit years into "2,005" + return MessageFormat.format(bundle.getString("in_year_x"), String.valueOf(((On) fieldExpression).getTime().getValue())); + } + return ""; + }); + return years; } /** diff --git a/src/main/java/com/cronutils/descriptor/NominalDescriptionStrategy.java b/src/main/java/com/cronutils/descriptor/NominalDescriptionStrategy.java index 9739f0e1..96b653ca 100644 --- a/src/main/java/com/cronutils/descriptor/NominalDescriptionStrategy.java +++ b/src/main/java/com/cronutils/descriptor/NominalDescriptionStrategy.java @@ -16,7 +16,7 @@ import com.cronutils.Function; import com.cronutils.model.field.expression.FieldExpression; -import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.ResourceBundle; import java.util.Set; @@ -41,7 +41,7 @@ class NominalDescriptionStrategy extends DescriptionStrategy { */ public NominalDescriptionStrategy(final ResourceBundle bundle, final Function nominalValueFunction, final FieldExpression expression) { super(bundle); - descriptions = new HashSet<>(); + descriptions = new LinkedHashSet<>(); if (nominalValueFunction != null) { this.nominalValueFunction = nominalValueFunction; } @@ -74,4 +74,14 @@ public NominalDescriptionStrategy addDescription(final Function Date: Tue, 28 Jul 2026 14:03:17 +0100 Subject: [PATCH 4/4] Read day of week as prose and match lists to it Day of week kept the generic template, so "0 10,44 14 ? 3 WED" read "... in March at Wednesday day" once months had been fixed. Give it the same treatment, so it reads "in March on Wednesday". Fixing the single value exposed the list form, which still read "at Monday, Wednesday and Friday days". Match lists to the single value they generalise across day of month, day of week, month and year, so a list and a single value do not use two different vocabularies in one sentence. A list reuses the key of its single value form, passing the joined values as the argument, so only day of month needs a plural key of its own. Ranges, every expressions and the L and # forms are untouched. Refs #3 --- .../DescriptionStrategyFactory.java | 51 ++++++++++++++++--- .../com/cronutils/CronUtilsI18N.properties | 2 + .../ExecutionTimeQuartzIntegrationTest.java | 2 +- .../CronDescriptorQuartzIntegrationTest.java | 2 +- .../utils/descriptor/Issue3Test.java | 27 ++++++++++ 5 files changed, 75 insertions(+), 9 deletions(-) diff --git a/src/main/java/com/cronutils/descriptor/DescriptionStrategyFactory.java b/src/main/java/com/cronutils/descriptor/DescriptionStrategyFactory.java index 942fdbcd..8d5ccf95 100755 --- a/src/main/java/com/cronutils/descriptor/DescriptionStrategyFactory.java +++ b/src/main/java/com/cronutils/descriptor/DescriptionStrategyFactory.java @@ -16,6 +16,7 @@ import com.cronutils.Function; import com.cronutils.model.field.definition.DayOfWeekFieldDefinition; import com.cronutils.model.field.definition.FieldDefinition; +import com.cronutils.model.field.expression.And; import com.cronutils.model.field.expression.Every; import com.cronutils.model.field.expression.FieldExpression; import com.cronutils.model.field.expression.On; @@ -25,6 +26,8 @@ import java.time.DayOfWeek; import java.time.Month; import java.time.format.TextStyle; +import java.util.ArrayList; +import java.util.List; import java.util.ResourceBundle; class DescriptionStrategyFactory { @@ -41,6 +44,34 @@ private static String ordinal(final int nth, final ResourceBundle bundle) { return bundle.containsKey(key) ? bundle.getString(key) : String.valueOf(nth); } + /** + * Joins a list of plain On values as "a, b and c", so a list can reuse the same phrasing + * as the single value case. + * + * @return the joined values, or null if the expression is not a list of at least two plain On values + */ + private static String joinPlainValues(final FieldExpression expression, final Function nominal, final ResourceBundle bundle) { + if (!(expression instanceof And)) { + return null; + } + final List values = new ArrayList<>(); + for (final FieldExpression each : ((And) expression).getExpressions()) { + if (!(each instanceof On) || ((On) each).getSpecialChar().getValue() != SpecialChar.NONE) { + return null; + } + values.add(nominal.apply(((On) each).getTime().getValue())); + } + if (values.size() < 2) { + return null; + } + final String last = values.remove(values.size() - 1); + return String.join(", ", values) + " " + bundle.getString("and") + " " + last; + } + + private static boolean isPlainOn(final FieldExpression expression) { + return expression instanceof On && ((On) expression).getSpecialChar().getValue() == SpecialChar.NONE; + } + /** * Creates description strategy for days of week. * @@ -68,11 +99,14 @@ public static DescriptionStrategy daysOfWeekInstance(final ResourceBundle bundle ordinal(on.getNth().getValue(), bundle), nominal.apply(on.getTime().getValue())); case L: return MessageFormat.format(bundle.getString("on_last_day_of_week_x"), nominal.apply(on.getTime().getValue())); + case NONE: + return MessageFormat.format(bundle.getString("on_day_of_week_x"), nominal.apply(on.getTime().getValue())); default: return ""; } } - return ""; + final String joined = joinPlainValues(fieldExpression, nominal, bundle); + return joined == null ? "" : MessageFormat.format(bundle.getString("on_day_of_week_x"), joined); }); return dow; } @@ -111,7 +145,8 @@ public static DescriptionStrategy daysOfMonthInstance(final ResourceBundle bundl return ""; } } - return ""; + final String joined = joinPlainValues(fieldExpression, Object::toString, bundle); + return joined == null ? "" : MessageFormat.format(bundle.getString("on_days_x"), joined); }); return dom; } @@ -129,10 +164,11 @@ public static DescriptionStrategy monthsInstance(final ResourceBundle bundle, fi new NominalDescriptionStrategy(bundle, nominal, expression).withSelfDescribingValues(); months.addDescription(fieldExpression -> { - if (fieldExpression instanceof On && ((On) fieldExpression).getSpecialChar().getValue() == SpecialChar.NONE) { + if (isPlainOn(fieldExpression)) { return MessageFormat.format(bundle.getString("in_month_x"), nominal.apply(((On) fieldExpression).getTime().getValue())); } - return ""; + final String joined = joinPlainValues(fieldExpression, nominal, bundle); + return joined == null ? "" : MessageFormat.format(bundle.getString("in_month_x"), joined); }); return months; } @@ -149,11 +185,12 @@ public static DescriptionStrategy yearsInstance(final ResourceBundle bundle, fin new NominalDescriptionStrategy(bundle, null, expression).withSelfDescribingValues(); years.addDescription(fieldExpression -> { - if (fieldExpression instanceof On && ((On) fieldExpression).getSpecialChar().getValue() == SpecialChar.NONE) { - // as a String, so MessageFormat does not group four digit years into "2,005" + // as Strings, so MessageFormat does not group four digit years into "2,005" + if (isPlainOn(fieldExpression)) { return MessageFormat.format(bundle.getString("in_year_x"), String.valueOf(((On) fieldExpression).getTime().getValue())); } - return ""; + final String joined = joinPlainValues(fieldExpression, String::valueOf, bundle); + return joined == null ? "" : MessageFormat.format(bundle.getString("in_year_x"), joined); }); return years; } diff --git a/src/main/resources/com/cronutils/CronUtilsI18N.properties b/src/main/resources/com/cronutils/CronUtilsI18N.properties index 99d88f7b..6d311619 100644 --- a/src/main/resources/com/cronutils/CronUtilsI18N.properties +++ b/src/main/resources/com/cronutils/CronUtilsI18N.properties @@ -19,6 +19,8 @@ between=between in_month_x=in {0} in_year_x=in {0} on_day_x=on day {0} +on_days_x=on days {0} +on_day_of_week_x=on {0} on_nth_day_of_week_x=on the {0} {1} of the month on_last_day_of_week_x=on the last {0} of the month nth_1=first diff --git a/src/test/java/com/cronutils/model/time/ExecutionTimeQuartzIntegrationTest.java b/src/test/java/com/cronutils/model/time/ExecutionTimeQuartzIntegrationTest.java index 0d7cda2d..376c1463 100755 --- a/src/test/java/com/cronutils/model/time/ExecutionTimeQuartzIntegrationTest.java +++ b/src/test/java/com/cronutils/model/time/ExecutionTimeQuartzIntegrationTest.java @@ -571,7 +571,7 @@ public void noDateTimeExceptionIsThrownGeneratingNextExecutionWithDayOfWeekFilte public void descriptionForExpressionTellsWrongDoW() { final CronDescriptor descriptor = CronDescriptor.instance(); final Cron quartzCron = parser.parse("0 0 8 ? * SUN *"); - assertEquals("at 08:00 at Sunday day", descriptor.describe(quartzCron)); + assertEquals("at 08:00 on Sunday", descriptor.describe(quartzCron)); } /** diff --git a/src/test/java/com/cronutils/utils/descriptor/CronDescriptorQuartzIntegrationTest.java b/src/test/java/com/cronutils/utils/descriptor/CronDescriptorQuartzIntegrationTest.java index 885dd0bb..d295eb01 100755 --- a/src/test/java/com/cronutils/utils/descriptor/CronDescriptorQuartzIntegrationTest.java +++ b/src/test/java/com/cronutils/utils/descriptor/CronDescriptorQuartzIntegrationTest.java @@ -123,7 +123,7 @@ public void testEveryDayEveryFourHoursFromHour2() { */ @Test public void testDescriptionDayOfWeek() { - assertExpression("* 0/1 * ? * TUE", "every second every minute at Tuesday day"); + assertExpression("* 0/1 * ? * TUE", "every second every minute on Tuesday"); } /** diff --git a/src/test/java/com/cronutils/utils/descriptor/Issue3Test.java b/src/test/java/com/cronutils/utils/descriptor/Issue3Test.java index ff2fc708..a7e7cb4f 100644 --- a/src/test/java/com/cronutils/utils/descriptor/Issue3Test.java +++ b/src/test/java/com/cronutils/utils/descriptor/Issue3Test.java @@ -79,6 +79,33 @@ public void aMeaningfulStartIsKept(String expression, String expected) { assertEquals(expected, CronDescriptor.instance(Locale.ENGLISH).describe(parser.parse(expression))); } + @ParameterizedTest + @CsvSource({ + "'0 0 0 ? * SUN', 'at 00:00 on Sunday'", + "'0 10,44 14 ? 3 WED', 'at 10 and 44 minutes at 14 hour in March on Wednesday'", + "'0 0 0 ? * MON-FRI', 'at 00:00 every day between Monday and Friday'", + "'0 0 0 ? * 6L', 'at 00:00 on the last Friday of the month'", + "'0 0 0 ? * 3/5', 'at 00:00 every 5 days from Tuesday'" + }) + public void dayOfWeekReadsAsProse(String expression, String expected) { + assertEquals(expected, CronDescriptor.instance(Locale.ENGLISH).describe(parser.parse(expression))); + } + + /** + * A list has to read the same way as the single value it generalises, else "in March" + * sits next to "at January, May and December months" in the same vocabulary. + */ + @ParameterizedTest + @CsvSource({ + "'0 0 16 * 1,5,12 ?', 'at 16:00 in January, May and December'", + "'0 0 16 1,5,26 * ?', 'at 16:00 on days 1, 5 and 26'", + "'0 0 0 ? * MON,WED,FRI', 'at 00:00 on Monday, Wednesday and Friday'", + "'0 15 10 * * ? 2005,2006','at 10:15 in 2005 and 2006'" + }) + public void listsReadLikeTheSingleValueForm(String expression, String expected) { + assertEquals(expected, CronDescriptor.instance(Locale.ENGLISH).describe(parser.parse(expression))); + } + /** * The expression from Naxos84's 2017 comment on issue 3, which described month 10 as * "month 10" rather than October and the year start as "year 2017".