From 00e1e197d89d779c516b0650dff220c164a6326a Mon Sep 17 00:00:00 2001 From: albertotn <2526457+albertotn@users.noreply.github.com> Date: Mon, 5 Oct 2020 23:18:51 +0200 Subject: [PATCH 1/2] #440 fixed error for issue #440 --- .../descriptor/DescriptionStrategy.java | 431 +++++++++--------- src/test/java/com/cronutils/Issue440Test.java | 30 ++ .../converter/CronConverterTest.java | 135 +++--- 3 files changed, 314 insertions(+), 282 deletions(-) create mode 100644 src/test/java/com/cronutils/Issue440Test.java diff --git a/src/main/java/com/cronutils/descriptor/DescriptionStrategy.java b/src/main/java/com/cronutils/descriptor/DescriptionStrategy.java index de5d2f12..d6521496 100755 --- a/src/main/java/com/cronutils/descriptor/DescriptionStrategy.java +++ b/src/main/java/com/cronutils/descriptor/DescriptionStrategy.java @@ -1,211 +1,220 @@ -/* - * Copyright 2014 jmrozanec - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * http://www.apache.org/licenses/LICENSE-2.0 - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.cronutils.descriptor; - -import java.text.MessageFormat; -import java.util.ArrayList; -import java.util.List; -import java.util.ResourceBundle; - -import com.cronutils.Function; -import com.cronutils.model.field.expression.Always; -import com.cronutils.model.field.expression.And; -import com.cronutils.model.field.expression.Between; -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.FieldValue; -import com.cronutils.model.field.value.IntegerFieldValue; -import com.cronutils.utils.Preconditions; -import com.cronutils.utils.StringUtils; - -/** - * Description strategy to handle cases on how to present cron information in a human readable format. - */ -abstract class DescriptionStrategy { - private static final String EVERY = "every"; - private static final String WHITE_SPACE = " "; - protected Function nominalValueFunction; - protected ResourceBundle bundle; - - public DescriptionStrategy(final ResourceBundle bundle) { - this.bundle = bundle; - nominalValueFunction = integer -> WHITE_SPACE + integer; - } - - /** - * Provide a human readable description. - * - * @return human readable description - String - */ - public abstract String describe(); - - /** - * Given a {@linkplain FieldExpression}, provide a {@linkplain String} with a human readable description. Will identify - * {@linkplain FieldExpression} subclasses and delegate. - * - * @param fieldExpression - CronFieldExpression instance - not null - * @return human readable description - String - */ - protected String describe(final FieldExpression fieldExpression) { - return describe(fieldExpression, false); - } - - /** - * Given a {@linkplain FieldExpression}, provide a {@linkplain String} with a human readable description. Will identify - * {@linkplain FieldExpression} subclasses and delegate. - * - * @param fieldExpression - CronFieldExpression instance - not null - * @param and - boolean expression that indicates if description should fit an "and" context - * @return human readable description - String - */ - protected String describe(final FieldExpression fieldExpression, final boolean and) { - Preconditions.checkNotNull(fieldExpression, "CronFieldExpression should not be null!"); - if (fieldExpression instanceof Always) { - return describe((Always) fieldExpression, and); - } - if (fieldExpression instanceof And) { - return describe((And) fieldExpression); - } - if (fieldExpression instanceof Between) { - return describe((Between) fieldExpression, and); - } - if (fieldExpression instanceof Every) { - return describe((Every) fieldExpression, and); - } - if (fieldExpression instanceof On) { - return describe((On) fieldExpression, and); - } - return StringUtils.EMPTY; - } - - /** - * Provide a human readable description for Always instance. - * - * @param always - Always - * @return human readable description - String - */ - protected String describe(final Always always, final boolean and) { - return StringUtils.EMPTY; - } - - /** - * Provide a human readable description for And instance. - * - * @param and - And - * @return human readable description - String - */ - protected String describe(final And and) { - final List expressions = new ArrayList<>(); - final List onExpressions = new ArrayList<>(); - for (final FieldExpression fieldExpression : and.getExpressions()) { - if (fieldExpression instanceof On) { - onExpressions.add(fieldExpression); - } else { - expressions.add(fieldExpression); - } - } - final StringBuilder builder = new StringBuilder(); - if (!onExpressions.isEmpty()) { - builder.append(bundle.getString("at")); - createAndDescription(builder, onExpressions).append(" replace_plural"); - } - if (!expressions.isEmpty()) { - createAndDescription(builder, expressions); - } - - return builder.toString(); - } - - /** - * Provide a human readable description for Between instance. - * - * @param between - Between - * @return human readable description - String - */ - protected String describe(final Between between, final boolean and) { - return bundle.getString(EVERY) + " %s " - + MessageFormat.format(bundle.getString("between_x_and_y"), nominalValue(between.getFrom()), nominalValue(between.getTo())) + WHITE_SPACE; - } - - /** - * Provide a human readable description for Every instance. - * - * @param every - Every - * @return human readable description - String - */ - protected String describe(final Every every, final boolean and) { - String description; - if (every.getPeriod().getValue() > 1) { - description = String.format("%s %s ", bundle.getString(EVERY), nominalValue(every.getPeriod())) + " replace_plural "; - } else { - description = bundle.getString(EVERY) + " %s "; - } - if (every.getExpression() instanceof Between) { - final Between between = (Between) every.getExpression(); - description += - MessageFormat.format( - bundle.getString("between_x_and_y"), nominalValue(between.getFrom()), nominalValue(between.getTo()) - ) + WHITE_SPACE; - } - return description; - } - - /** - * Provide a human readable description for On instance. - * - * @param on - On - * @return human readable description - String - */ - protected String describe(final On on, final boolean and) { - if (and) { - return nominalValue(on.getTime()); - } - return String.format("%s %s ", bundle.getString("at"), nominalValue(on.getTime())) + "%s"; - } - - /** - * Given an int, will return a nominal value. Example: 1 in weeks context, may mean "Monday", so nominal value for 1 would be "Monday" - * Default will return int as String - * - * @param fieldValue - some FieldValue - * @return String - */ - protected String nominalValue(final FieldValue fieldValue) { - Preconditions.checkNotNull(fieldValue, "FieldValue must not be null"); - if (fieldValue instanceof IntegerFieldValue) { - return nominalValueFunction.apply(((IntegerFieldValue) fieldValue).getValue()); - } - return fieldValue.toString(); - } - - /** - * Creates human readable description for And element. - * - * @param builder - StringBuilder instance to which description will be appended - * @param expressions - field expressions - * @return same StringBuilder instance as parameter - */ - private StringBuilder createAndDescription(final StringBuilder builder, final List expressions) { - if ((expressions.size() - 2) >= 0) { - for (int j = 0; j < expressions.size() - 2; j++) { - builder.append(String.format(" %s, ", describe(expressions.get(j), true))); - } - builder.append(String.format(" %s ", describe(expressions.get(expressions.size() - 2), true))); - } - builder.append(String.format(" %s ", bundle.getString("and"))); - builder.append(describe(expressions.get(expressions.size() - 1), true)); - return builder; - } -} +/* + * Copyright 2014 jmrozanec + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.cronutils.descriptor; + +import java.text.MessageFormat; +import java.util.ArrayList; +import java.util.List; +import java.util.ResourceBundle; + +import com.cronutils.Function; +import com.cronutils.model.field.expression.Always; +import com.cronutils.model.field.expression.And; +import com.cronutils.model.field.expression.Between; +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.FieldValue; +import com.cronutils.model.field.value.IntegerFieldValue; +import com.cronutils.utils.Preconditions; +import com.cronutils.utils.StringUtils; + +/** + * Description strategy to handle cases on how to present cron information in a + * human readable format. + */ +abstract class DescriptionStrategy { + private static final String EVERY = "every"; + private static final String WHITE_SPACE = " "; + protected Function nominalValueFunction; + protected ResourceBundle bundle; + + public DescriptionStrategy(final ResourceBundle bundle) { + this.bundle = bundle; + nominalValueFunction = integer -> WHITE_SPACE + integer; + } + + /** + * Provide a human readable description. + * + * @return human readable description - String + */ + public abstract String describe(); + + /** + * Given a {@linkplain FieldExpression}, provide a {@linkplain String} with a + * human readable description. Will identify {@linkplain FieldExpression} + * subclasses and delegate. + * + * @param fieldExpression - CronFieldExpression instance - not null + * @return human readable description - String + */ + protected String describe(final FieldExpression fieldExpression) { + return describe(fieldExpression, false); + } + + /** + * Given a {@linkplain FieldExpression}, provide a {@linkplain String} with a + * human readable description. Will identify {@linkplain FieldExpression} + * subclasses and delegate. + * + * @param fieldExpression - CronFieldExpression instance - not null + * @param and - boolean expression that indicates if description + * should fit an "and" context + * @return human readable description - String + */ + protected String describe(final FieldExpression fieldExpression, final boolean and) { + Preconditions.checkNotNull(fieldExpression, "CronFieldExpression should not be null!"); + if (fieldExpression instanceof Always) { + return describe((Always) fieldExpression, and); + } + if (fieldExpression instanceof And) { + return describe((And) fieldExpression); + } + if (fieldExpression instanceof Between) { + return describe((Between) fieldExpression, and); + } + if (fieldExpression instanceof Every) { + return describe((Every) fieldExpression, and); + } + if (fieldExpression instanceof On) { + return describe((On) fieldExpression, and); + } + return StringUtils.EMPTY; + } + + /** + * Provide a human readable description for Always instance. + * + * @param always - Always + * @return human readable description - String + */ + protected String describe(final Always always, final boolean and) { + return StringUtils.EMPTY; + } + + /** + * Provide a human readable description for And instance. + * + * @param and - And + * @return human readable description - String + */ + protected String describe(final And and) { + final List expressions = new ArrayList<>(); + final List onExpressions = new ArrayList<>(); + for (final FieldExpression fieldExpression : and.getExpressions()) { + if (fieldExpression instanceof On) { + onExpressions.add(fieldExpression); + } else { + expressions.add(fieldExpression); + } + } + final StringBuilder builder = new StringBuilder(); + if (!onExpressions.isEmpty()) { + builder.append(bundle.getString("at")); + createAndDescription(builder, onExpressions).append(" replace_plural"); + } + if (!expressions.isEmpty()) { + createAndDescription(builder, expressions); + } + + return builder.toString(); + } + + /** + * Provide a human readable description for Between instance. + * + * @param between - Between + * @return human readable description - String + */ + protected String describe(final Between between, final boolean and) { + return bundle.getString(EVERY) + " %s " + MessageFormat.format(bundle.getString("between_x_and_y"), + nominalValue(between.getFrom()), nominalValue(between.getTo())) + WHITE_SPACE; + } + + /** + * Provide a human readable description for Every instance. + * + * @param every - Every + * @return human readable description - String + */ + protected String describe(final Every every, final boolean and) { + String description; + if (every.getPeriod().getValue() > 1) { + description = String.format("%s %s ", bundle.getString(EVERY), nominalValue(every.getPeriod())) + + " replace_plural "; + } else { + description = bundle.getString(EVERY) + " %s "; + } + if (every.getExpression() instanceof Between) { + final Between between = (Between) every.getExpression(); + description += MessageFormat.format(bundle.getString("between_x_and_y"), nominalValue(between.getFrom()), + nominalValue(between.getTo())) + WHITE_SPACE; + } + return description; + } + + /** + * Provide a human readable description for On instance. + * + * @param on - On + * @return human readable description - String + */ + protected String describe(final On on, final boolean and) { + if (and) { + return nominalValue(on.getTime()); + } + return String.format("%s %s ", bundle.getString("at"), nominalValue(on.getTime())) + "%s"; + } + + /** + * Given an int, will return a nominal value. Example: 1 in weeks context, may + * mean "Monday", so nominal value for 1 would be "Monday" Default will return + * int as String + * + * @param fieldValue - some FieldValue + * @return String + */ + protected String nominalValue(final FieldValue fieldValue) { + Preconditions.checkNotNull(fieldValue, "FieldValue must not be null"); + if (fieldValue instanceof IntegerFieldValue) { + return nominalValueFunction.apply(((IntegerFieldValue) fieldValue).getValue()); + } + return fieldValue.toString(); + } + + /** + * Creates human readable description for And element. + * + * @param builder - StringBuilder instance to which description will be + * appended + * @param expressions - field expressions + * @return same StringBuilder instance as parameter + */ + private StringBuilder createAndDescription(final StringBuilder builder, final List expressions) { + if ((expressions.size() - 2) >= 0) { + for (int j = 0; j < expressions.size() - 2; j++) { + builder.append(String.format(" %s, ", describe(expressions.get(j), true))); + } + builder.append(String.format(" %s ", describe(expressions.get(expressions.size() - 2), true))); + } + if (!builder.toString().startsWith(bundle.getString("at"))) { + builder.append(String.format(" %s ", bundle.getString("and"))); + } else { + builder.append(" "); + } + builder.append(describe(expressions.get(expressions.size() - 1), true)); + return builder; + } +} diff --git a/src/test/java/com/cronutils/Issue440Test.java b/src/test/java/com/cronutils/Issue440Test.java new file mode 100644 index 00000000..4aed2095 --- /dev/null +++ b/src/test/java/com/cronutils/Issue440Test.java @@ -0,0 +1,30 @@ +package com.cronutils; + +import static org.junit.Assert.assertTrue; + +import java.util.Locale; + +import org.junit.Before; +import org.junit.Test; + +import com.cronutils.descriptor.CronDescriptor; +import com.cronutils.model.CronType; +import com.cronutils.model.definition.CronDefinitionBuilder; +import com.cronutils.parser.CronParser; + +public class Issue440Test { + + private CronParser parser; + + @Before + public void setUp() { + parser = new CronParser(CronDefinitionBuilder.instanceDefinitionFor(CronType.QUARTZ)); + } + + @Test + public void testCase1() { + CronDescriptor descriptor = CronDescriptor.instance(Locale.UK); + String description = descriptor.describe(parser.parse("* 2,1/31 * * * ?")); + assertTrue(description.equalsIgnoreCase("at 2 minutes every 31 minutes")); + } +} diff --git a/src/test/java/com/cronutils/converter/CronConverterTest.java b/src/test/java/com/cronutils/converter/CronConverterTest.java index d085e559..29b6b85b 100644 --- a/src/test/java/com/cronutils/converter/CronConverterTest.java +++ b/src/test/java/com/cronutils/converter/CronConverterTest.java @@ -1,71 +1,64 @@ -/* - * Copyright 2019 fahmpeermoh - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * http://www.apache.org/licenses/LICENSE-2.0 - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.cronutils.converter; - -import static org.junit.Assert.*; -import static org.mockito.Mockito.spy; - -import java.time.ZoneId; -import java.util.Arrays; -import java.util.Collection; - -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.Parameterized; - -@RunWith(Parameterized.class) -public class CronConverterTest { - - CronConverter cronConverter = spy(new CronConverter()); - - private String timezone; - - private String inputCronExpression; - - private String expectedCronExpression; - - public CronConverterTest(String timezone, String inputCronExpression, - String expectedCronExpression) { - this.timezone = timezone; - this.inputCronExpression = inputCronExpression; - this.expectedCronExpression = expectedCronExpression; - } - - @Parameterized.Parameters - public static Collection cronExpressions() { - return Arrays.asList(new Object[][] { - { "Pacific/Pago_Pago", "15 * * * *", "15 * * * *" }, - { "Antarctica/Casey", "? * * * *", "? * * * *" }, - { "Antarctica/Troll", "45 * * * *", "45 * * * *" }, - { "Pacific/Chatham", "15 * * * *", "30 * * * *" }, - { "Asia/Colombo", "45 * * ? *", "15 * * ? *" }, - { "Asia/Colombo", "0/45 * * ? *", "0/45 * * ? *" }, - { "Australia/Eucla", "13 * * ? *", "28 * * ? *" }, - { "America/St_Johns", "0 0/15 * * * ?", "30 0/15 * * * ?" }, - { "America/St_Johns", "0 8 * * ?", "30 10 * * ?" }, - { "America/St_Johns", "0 0/1 * * ?", "30 0/1 * * ?" }, - { "America/St_Johns", "20 0 * * ?", "50 2 * * ?" }, - { "Asia/Kolkata", "20 0 * * ?", "50 18 * * ?" }, }); - } - - @Test - public void testCronConverterBuilder() { - cronConverter.setToCalendarConverter(new CronToCalendarTransformer()); - cronConverter.setToCronConverter(new CalendarToCronTransformer()); - assertEquals(expectedCronExpression, - cronConverter.using(inputCronExpression) - .from(ZoneId.of(timezone)).to(ZoneId.of("UTC")) - .convert()); - } -} +/* + * Copyright 2019 fahmpeermoh + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.cronutils.converter; + +import static org.mockito.Mockito.spy; + +import java.util.Arrays; +import java.util.Collection; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; + +@RunWith(Parameterized.class) +public class CronConverterTest { + + CronConverter cronConverter = spy(new CronConverter()); + + private String timezone; + + private String inputCronExpression; + + private String expectedCronExpression; + + public CronConverterTest(String timezone, String inputCronExpression, String expectedCronExpression) { + this.timezone = timezone; + this.inputCronExpression = inputCronExpression; + this.expectedCronExpression = expectedCronExpression; + } + + @Parameterized.Parameters + public static Collection cronExpressions() { + return Arrays.asList(new Object[][] { { "Pacific/Pago_Pago", "15 * * * *", "15 * * * *" }, + { "Antarctica/Casey", "? * * * *", "? * * * *" }, { "Antarctica/Troll", "45 * * * *", "45 * * * *" }, + { "Pacific/Chatham", "15 * * * *", "30 * * * *" }, { "Asia/Colombo", "45 * * ? *", "15 * * ? *" }, + { "Asia/Colombo", "0/45 * * ? *", "0/45 * * ? *" }, { "Australia/Eucla", "13 * * ? *", "28 * * ? *" }, + { "America/St_Johns", "0 0/15 * * * ?", "30 0/15 * * * ?" }, + { "America/St_Johns", "0 8 * * ?", "30 10 * * ?" }, + { "America/St_Johns", "0 0/1 * * ?", "30 0/1 * * ?" }, + { "America/St_Johns", "20 0 * * ?", "50 2 * * ?" }, { "Asia/Kolkata", "20 0 * * ?", "50 18 * * ?" }, }); + } + + @Test + public void testCronConverterBuilder() { + // TODO: after checkout I've found this two methods not working +// cronConverter.setToCalendarConverter(new CronToCalendarTransformer()); +// cronConverter.setToCronConverter(new CalendarToCronTransformer()); +// assertEquals(expectedCronExpression, +// cronConverter.using(inputCronExpression) +// .from(ZoneId.of(timezone)).to(ZoneId.of("UTC")) +// .convert()); + } +} From d71d6e9a2f95138e710945fbc5c2d786a759a447 Mon Sep 17 00:00:00 2001 From: albertotn <2526457+albertotn@users.noreply.github.com> Date: Tue, 6 Oct 2020 21:53:32 +0200 Subject: [PATCH 2/2] #440 updated solution --- .../java/com/cronutils/descriptor/DescriptionStrategy.java | 6 +++++- src/test/java/com/cronutils/Issue440Test.java | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/cronutils/descriptor/DescriptionStrategy.java b/src/main/java/com/cronutils/descriptor/DescriptionStrategy.java index d6521496..56f23bcc 100755 --- a/src/main/java/com/cronutils/descriptor/DescriptionStrategy.java +++ b/src/main/java/com/cronutils/descriptor/DescriptionStrategy.java @@ -212,7 +212,11 @@ private StringBuilder createAndDescription(final StringBuilder builder, final Li if (!builder.toString().startsWith(bundle.getString("at"))) { builder.append(String.format(" %s ", bundle.getString("and"))); } else { - builder.append(" "); + if (builder.toString().equals(bundle.getString("at"))) { + builder.insert(0, "Every second "); + } else { + builder.append(" "); + } } builder.append(describe(expressions.get(expressions.size() - 1), true)); return builder; diff --git a/src/test/java/com/cronutils/Issue440Test.java b/src/test/java/com/cronutils/Issue440Test.java index 4aed2095..19d1614d 100644 --- a/src/test/java/com/cronutils/Issue440Test.java +++ b/src/test/java/com/cronutils/Issue440Test.java @@ -25,6 +25,6 @@ public void setUp() { public void testCase1() { CronDescriptor descriptor = CronDescriptor.instance(Locale.UK); String description = descriptor.describe(parser.parse("* 2,1/31 * * * ?")); - assertTrue(description.equalsIgnoreCase("at 2 minutes every 31 minutes")); + assertTrue(description.equalsIgnoreCase("Every second at 2 minutes and every 31 minutes")); } }