Index: /trunk/src/org/openstreetmap/josm/data/validation/tests/ConditionalKeys.java =================================================================== --- /trunk/src/org/openstreetmap/josm/data/validation/tests/ConditionalKeys.java (revision 16085) +++ /trunk/src/org/openstreetmap/josm/data/validation/tests/ConditionalKeys.java (revision 16086) @@ -9,5 +9,4 @@ import java.util.HashSet; import java.util.List; -import java.util.Locale; import java.util.Set; import java.util.regex.Matcher; @@ -204,8 +203,7 @@ for (final String condition : conditional.conditions) { if (condition.matches(".*[0-9]:[0-9]{2}.*")) { - final List errors = openingHourTest.checkOpeningHourSyntax( - "", condition, true, Locale.getDefault()); + final List errors = openingHourTest.checkOpeningHourSyntax("", condition); if (!errors.isEmpty()) { - return errors.get(0).getMessage(); + return errors.get(0).getDescription(); } } Index: /trunk/src/org/openstreetmap/josm/data/validation/tests/OpeningHourTest.java =================================================================== --- /trunk/src/org/openstreetmap/josm/data/validation/tests/OpeningHourTest.java (revision 16085) +++ /trunk/src/org/openstreetmap/josm/data/validation/tests/OpeningHourTest.java (revision 16086) @@ -5,4 +5,6 @@ import java.io.StringReader; +import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.List; @@ -29,4 +31,6 @@ public class OpeningHourTest extends TagTest { + private static final Collection KEYS_TO_CHECK = Arrays.asList("opening_hours", "collection_times", "service_times"); + /** * Constructs a new {@code OpeningHourTest}. @@ -38,67 +42,20 @@ /** - * An error concerning invalid syntax for an "opening_hours"-like tag. + * Returns the real test error given to JOSM validator. + * @param severity The error severity + * @param message The error message + * @param key The incriminated key, used for display. + * @param prettifiedValue The prettified value + * @param p The incriminated OSM primitive. + * @return The real test error given to JOSM validator. Can be fixable or not if a prettified values has been determined. */ - public class OpeningHoursTestError { - private final Severity severity; - private final String message; - private final String prettifiedValue; - - /** - * Constructs a new {@code OpeningHoursTestError} with a known prettified value. - * @param message The error message - * @param severity The error severity - * @param prettifiedValue The prettified value - */ - public OpeningHoursTestError(String message, Severity severity, String prettifiedValue) { - this.message = message; - this.severity = severity; - this.prettifiedValue = prettifiedValue; - } - - /** - * Returns the real test error given to JOSM validator. - * @param p The incriminated OSM primitive. - * @param key The incriminated key, used for display. - * @return The real test error given to JOSM validator. Can be fixable or not if a prettified values has been determined. - */ - public TestError getTestError(final OsmPrimitive p, final String key) { - final TestError.Builder error = TestError.builder(OpeningHourTest.this, severity, 2901) - .message(tr("Opening hours syntax"), message) // todo obtain English message for ignore functionality - .primitives(p); - if (prettifiedValue == null || prettifiedValue.equals(p.get(key))) { - return error.build(); - } else { - return error.fix(() -> new ChangePropertyCommand(p, key, prettifiedValue)).build(); - } - } - - /** - * Returns the error message. - * @return The error message. - */ - public String getMessage() { - return message; - } - - /** - * Returns the prettified value. - * @return The prettified value. - */ - public String getPrettifiedValue() { - return prettifiedValue; - } - - /** - * Returns the error severity. - * @return The error severity. - */ - public Severity getSeverity() { - return severity; - } - - @Override - public String toString() { - return getMessage() + " => " + getPrettifiedValue(); + private TestError createTestError(Severity severity, String message, String key, String prettifiedValue, OsmPrimitive p) { + final TestError.Builder error = TestError.builder(this, severity, 2901) + .message(tr("Opening hours syntax"), message) // todo obtain English message for ignore functionality + .primitives(p); + if (prettifiedValue == null || prettifiedValue.equals(p.get(key))) { + return error.build(); + } else { + return error.fix(() -> new ChangePropertyCommand(p, key, prettifiedValue)).build(); } } @@ -111,6 +68,6 @@ * @return a list of {@link TestError} or an empty list */ - public List checkOpeningHourSyntax(final String key, final String value) { - return checkOpeningHourSyntax(key, value, false, Locale.getDefault()); + public List checkOpeningHourSyntax(final String key, final String value) { + return checkOpeningHourSyntax(key, value, null, Locale.getDefault()); } @@ -120,9 +77,9 @@ * @param key the OSM key (should be "opening_hours", "collection_times" or "service_times"). * @param value the opening hour value to be checked. - * @param ignoreOtherSeverity whether to ignore errors with {@link Severity#OTHER}. + * @param p the primitive to check/fix. * @param locale the locale code used for localizing messages * @return a list of {@link TestError} or an empty list */ - public List checkOpeningHourSyntax(final String key, final String value, boolean ignoreOtherSeverity, Locale locale) { + List checkOpeningHourSyntax(final String key, final String value, OsmPrimitive p, Locale locale) { if (value == null || value.isEmpty()) { return Collections.emptyList(); @@ -139,18 +96,12 @@ } } catch (ParseException e) { - return Collections.singletonList(new OpeningHoursTestError(e.getMessage(), Severity.WARNING, prettifiedValue)); + return Collections.singletonList(createTestError(Severity.WARNING, e.getMessage(), key, prettifiedValue, p)); } - if (ignoreOtherSeverity || Objects.equals(value, prettifiedValue)) { + if (!includeOtherSeverityChecks() || Objects.equals(value, prettifiedValue) || p == null) { return Collections.emptyList(); } else { - return Collections.singletonList( - new OpeningHoursTestError(tr("{0} value can be prettified", key), Severity.OTHER, prettifiedValue)); - } - } - - protected void check(final OsmPrimitive p, final String key) { - for (OpeningHoursTestError e : checkOpeningHourSyntax(key, p.get(key))) { - errors.add(e.getTestError(p, key)); + final String message = tr("{0} value can be prettified", key); + return Collections.singletonList(createTestError(Severity.OTHER, message, key, prettifiedValue, p)); } } @@ -158,7 +109,7 @@ @Override public void check(final OsmPrimitive p) { - check(p, "opening_hours"); - check(p, "collection_times"); - check(p, "service_times"); + for (String key : KEYS_TO_CHECK) { + errors.addAll(checkOpeningHourSyntax(key, p.get(key), p, Locale.getDefault())); + } } } Index: /trunk/test/unit/org/openstreetmap/josm/data/validation/tests/OpeningHourTestTest.java =================================================================== --- /trunk/test/unit/org/openstreetmap/josm/data/validation/tests/OpeningHourTestTest.java (revision 16085) +++ /trunk/test/unit/org/openstreetmap/josm/data/validation/tests/OpeningHourTestTest.java (revision 16086) @@ -6,4 +6,5 @@ import static org.hamcrest.CoreMatchers.not; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; @@ -19,6 +20,12 @@ import org.junit.Rule; import org.junit.Test; +import org.openstreetmap.josm.command.ChangePropertyCommand; +import org.openstreetmap.josm.data.coor.LatLon; +import org.openstreetmap.josm.data.osm.DataSet; +import org.openstreetmap.josm.data.osm.Node; import org.openstreetmap.josm.data.osm.Tag; +import org.openstreetmap.josm.data.preferences.sources.ValidatorPrefHelper; import org.openstreetmap.josm.data.validation.Severity; +import org.openstreetmap.josm.data.validation.TestError; import org.openstreetmap.josm.gui.tagging.presets.TaggingPreset; import org.openstreetmap.josm.gui.tagging.presets.TaggingPresetItem; @@ -36,5 +43,5 @@ public class OpeningHourTestTest { /** - * We need prefs for this. We check strings so we need i18n. + * We need preferences for this. We check strings so we need i18n. */ @Rule @@ -52,4 +59,5 @@ openingHourTest = new OpeningHourTest(); openingHourTest.initialize(); + ValidatorPrefHelper.PREF_OTHER.put(true); } @@ -61,9 +69,9 @@ final String key = "opening_hours"; // frequently used tags according to https://taginfo.openstreetmap.org/keys/opening_hours#values - assertThat(openingHourTest.checkOpeningHourSyntax(key, "24/7"), isEmpty()); - assertThat(openingHourTest.checkOpeningHourSyntax(key, "Mo-Fr 08:30-20:00"), isEmpty()); - assertThat(openingHourTest.checkOpeningHourSyntax(key, "Mo-Fr sunrise-sunset"), isEmpty()); - assertThat(openingHourTest.checkOpeningHourSyntax(key, "09:00-21:00"), isEmpty()); - assertThat(openingHourTest.checkOpeningHourSyntax(key, "Su-Th sunset-24:00,04:00-sunrise; Fr-Sa sunset-sunrise"), isEmpty()); + assertThat(checkOpeningHourSyntax(key, "24/7"), isEmpty()); + assertThat(checkOpeningHourSyntax(key, "Mo-Fr 08:30-20:00"), isEmpty()); + assertThat(checkOpeningHourSyntax(key, "Mo-Fr sunrise-sunset"), isEmpty()); + assertThat(checkOpeningHourSyntax(key, "09:00-21:00"), isEmpty()); + assertThat(checkOpeningHourSyntax(key, "Su-Th sunset-24:00,04:00-sunrise; Fr-Sa sunset-sunrise"), isEmpty()); } @@ -75,13 +83,13 @@ final String key = "opening_hours"; String value = "."; - assertEquals("Vorgefunden wurde: \".\" \". \" in Zeile 0, Zeichen 0\nErwartet wurde: => null", - openingHourTest.checkOpeningHourSyntax(key, value, false, Locale.GERMAN).get(0).toString()); - assertEquals("Encountered: \".\" \". \" at line 0, column 0\nWas expecting: => null", - openingHourTest.checkOpeningHourSyntax(key, value, false, Locale.ENGLISH).get(0).toString()); + assertEquals("Vorgefunden wurde: \".\" \". \" in Zeile 0, Zeichen 0\nErwartet wurde: ", + checkOpeningHourSyntax(key, value, Locale.GERMAN).get(0).getDescription()); + assertEquals("Encountered: \".\" \". \" at line 0, column 0\nWas expecting: ", + checkOpeningHourSyntax(key, value, Locale.ENGLISH).get(0).getDescription()); value = "Mon-Thu 12-18"; - assertEquals("Wochentag mit 3 Buchstaben in Zeile 1, Zeichen 4 => Mo-Th 12:00-18:00", - openingHourTest.checkOpeningHourSyntax(key, value, false, Locale.GERMAN).get(0).toString()); - assertEquals("Three character weekday at line 1, column 4 => Mo-Th 12:00-18:00", - openingHourTest.checkOpeningHourSyntax(key, value, false, Locale.ENGLISH).get(0).toString()); + assertEquals("Wochentag mit 3 Buchstaben in Zeile 1, Zeichen 4", + checkOpeningHourSyntax(key, value, Locale.GERMAN).get(0).getDescription()); + assertEquals("Three character weekday at line 1, column 4", + checkOpeningHourSyntax(key, value, Locale.ENGLISH).get(0).getDescription()); } @@ -92,8 +100,8 @@ public void testCheckOpeningHourSyntax2() { final String key = "opening_hours"; - final List errors = openingHourTest.checkOpeningHourSyntax(key, "Mo-Tue"); + final List errors = checkOpeningHourSyntax(key, "Mo-Tue"); assertThat(errors, hasSize(1)); - assertEquals("Mo-Tu", errors.get(0).getPrettifiedValue()); - assertEquals("Three character weekday at line 1, column 6", errors.get(0).getMessage()); + assertFixEquals("Mo-Tu", errors.get(0)); + assertEquals("Three character weekday at line 1, column 6", errors.get(0).getDescription()); assertEquals(Severity.WARNING, errors.get(0).getSeverity()); } @@ -105,8 +113,8 @@ public void testCheckOpeningHourSyntax3() { final String key = "opening_hours"; - final List errors = openingHourTest.checkOpeningHourSyntax(key, "Sa-Su 10.00-20.00"); + final List errors = checkOpeningHourSyntax(key, "Sa-Su 10.00-20.00"); assertThat(errors, hasSize(1)); - assertEquals("Sa-Su 10:00-20:00", errors.get(0).getPrettifiedValue()); - assertEquals("Invalid minutes at line 1, column 12", errors.get(0).getMessage()); + assertFixEquals("Sa-Su 10:00-20:00", errors.get(0)); + assertEquals("Invalid minutes at line 1, column 12", errors.get(0).getDescription()); assertEquals(Severity.WARNING, errors.get(0).getSeverity()); } @@ -117,10 +125,10 @@ @Test public void testCheckOpeningHourSyntax4() { - assertThat(openingHourTest.checkOpeningHourSyntax(null, null), isEmpty()); - assertThat(openingHourTest.checkOpeningHourSyntax(null, ""), isEmpty()); + assertThat(checkOpeningHourSyntax(null, null), isEmpty()); + assertThat(checkOpeningHourSyntax(null, ""), isEmpty()); assertEquals("opening_hours value can be prettified", - openingHourTest.checkOpeningHourSyntax("opening_hours", " ").get(0).getMessage()); + checkOpeningHourSyntax("opening_hours", " ").get(0).getDescription()); assertEquals("null value can be prettified", - openingHourTest.checkOpeningHourSyntax(null, " ").get(0).getMessage()); + checkOpeningHourSyntax(null, " ").get(0).getDescription()); } @@ -131,10 +139,10 @@ public void testCheckOpeningHourSyntax5() { final String key = "opening_hours"; - assertThat(openingHourTest.checkOpeningHourSyntax(key, "badtext"), hasSize(1)); + assertThat(checkOpeningHourSyntax(key, "badtext"), hasSize(1)); assertEquals("Encountered: \"b \" at line 0, column 0\nWas expecting: ", - openingHourTest.checkOpeningHourSyntax(key, "badtext").get(0).getMessage().trim()); - assertThat(openingHourTest.checkOpeningHourSyntax(key, "5.00 p.m-11.00 p.m"), hasSize(1)); + checkOpeningHourSyntax(key, "badtext").get(0).getDescription().trim()); + assertThat(checkOpeningHourSyntax(key, "5.00 p.m-11.00 p.m"), hasSize(1)); assertEquals("Encountered: \"p \" at line 1, column 2\nWas expecting: ", - openingHourTest.checkOpeningHourSyntax(key, "5.00 p.m-11.00 p.m").get(0).getMessage()); + checkOpeningHourSyntax(key, "5.00 p.m-11.00 p.m").get(0).getDescription()); } @@ -145,5 +153,5 @@ public void testCheckOpeningHourSyntax6() { final String key = "opening_hours"; - assertThat(openingHourTest.checkOpeningHourSyntax(key, "PH open \"always open on public holidays\""), isEmpty()); + assertThat(checkOpeningHourSyntax(key, "PH open \"always open on public holidays\""), isEmpty()); } @@ -154,8 +162,7 @@ public void testCheckOpeningHourSyntax7() { final String key = "opening_hours"; - assertThat(openingHourTest.checkOpeningHourSyntax(key, "9:00-18:00", true, Locale.getDefault()), isEmpty()); - assertThat(openingHourTest.checkOpeningHourSyntax(key, "9:00-18:00"), hasSize(1)); - assertEquals(Severity.OTHER, openingHourTest.checkOpeningHourSyntax(key, "9:00-18:00").get(0).getSeverity()); - assertEquals("09:00-18:00", openingHourTest.checkOpeningHourSyntax(key, "9:00-18:00").get(0).getPrettifiedValue()); + assertThat(checkOpeningHourSyntax(key, "9:00-18:00"), hasSize(1)); + assertEquals(Severity.OTHER, checkOpeningHourSyntax(key, "9:00-18:00").get(0).getSeverity()); + assertFixEquals("09:00-18:00", checkOpeningHourSyntax(key, "9:00-18:00").get(0)); } @@ -166,7 +173,7 @@ public void testCheckOpeningHourSyntaxTicket9367() { final String key = "opening_hours"; - assertEquals(Severity.WARNING, openingHourTest.checkOpeningHourSyntax(key, "Mo,Tu 04-17").get(0).getSeverity()); + assertEquals(Severity.WARNING, checkOpeningHourSyntax(key, "Mo,Tu 04-17").get(0).getSeverity()); assertEquals("Hours without minutes", - openingHourTest.checkOpeningHourSyntax(key, "Mo,Tu 04-17").get(0).getMessage()); + checkOpeningHourSyntax(key, "Mo,Tu 04-17").get(0).getDescription()); } @@ -178,14 +185,14 @@ final String key = "service_times"; // frequently used tags according to https://taginfo.openstreetmap.org/keys/service_times#values - assertThat(openingHourTest.checkOpeningHourSyntax(key, "Su 10:00"), isEmpty()); - assertThat(openingHourTest.checkOpeningHourSyntax(key, "automatic"), not(isEmpty())); - assertThat(openingHourTest.checkOpeningHourSyntax(key, "Mo-Sa 09:00-18:00"), isEmpty()); - assertThat(openingHourTest.checkOpeningHourSyntax(key, "Su 09:30; We 19:30"), isEmpty()); - // assertThat(openingHourTest.checkOpeningHourSyntax(key, "Mo-Fr 00:00-00:30,04:00-00:30; Sa,Su,PH 00:00-24:00"), isEmpty()); - assertThat(openingHourTest.checkOpeningHourSyntax(key, "Mo-Fr 0:00-0:30,4:00-00:30; Sa,Su,PH 0:00-24:00"), hasSize(1)); - assertEquals("Mo-Fr 00:00-00:30,04:00-00:30; PH,Sa,Su 00:00-24:00", - openingHourTest.checkOpeningHourSyntax(key, "Mo-Fr 0:00-0:30,4:00-00:30; Sa,Su,PH 0:00-24:00").get(0).getPrettifiedValue()); - assertEquals("Mo-Fr 00:00-00:30,04:00-00:30; PH,Sa,Su 00:00-24:00", - openingHourTest.checkOpeningHourSyntax(key, "Mo-Fr 0:00-0:30,4:00-00:30; Sa,Su,PH 0:00-24:00").get(0).getPrettifiedValue()); + assertThat(checkOpeningHourSyntax(key, "Su 10:00"), isEmpty()); + assertThat(checkOpeningHourSyntax(key, "automatic"), not(isEmpty())); + assertThat(checkOpeningHourSyntax(key, "Mo-Sa 09:00-18:00"), isEmpty()); + assertThat(checkOpeningHourSyntax(key, "Su 09:30; We 19:30"), isEmpty()); + // assertThat(checkOpeningHourSyntax(key, "Mo-Fr 00:00-00:30,04:00-00:30; Sa,Su,PH 00:00-24:00"), isEmpty()); + assertThat(checkOpeningHourSyntax(key, "Mo-Fr 0:00-0:30,4:00-00:30; Sa,Su,PH 0:00-24:00"), hasSize(1)); + assertFixEquals("Mo-Fr 00:00-00:30,04:00-00:30; PH,Sa,Su 00:00-24:00", + checkOpeningHourSyntax(key, "Mo-Fr 0:00-0:30,4:00-00:30; Sa,Su,PH 0:00-24:00").get(0)); + assertFixEquals("Mo-Fr 00:00-00:30,04:00-00:30; PH,Sa,Su 00:00-24:00", + checkOpeningHourSyntax(key, "Mo-Fr 0:00-0:30,4:00-00:30; Sa,Su,PH 0:00-24:00").get(0)); } @@ -197,13 +204,13 @@ final String key = "collection_times"; // frequently used tags according to https://taginfo.openstreetmap.org/keys/collection_times#values - assertThat(openingHourTest.checkOpeningHourSyntax(key, "Mo-Sa 09:00"), isEmpty()); - assertThat(openingHourTest.checkOpeningHourSyntax(key, "fixme"), not(isEmpty())); - assertThat(openingHourTest.checkOpeningHourSyntax(key, "daily"), not(isEmpty())); - assertThat(openingHourTest.checkOpeningHourSyntax(key, "Mo-Fr 13:30,17:45,19:00; Sa 15:00; Su 11:00"), isEmpty()); - assertThat(openingHourTest.checkOpeningHourSyntax(key, "Mo-Fr 13:30, 17:45, 19:00; Sa 15:00; Su 11:00"), hasSize(1)); + assertThat(checkOpeningHourSyntax(key, "Mo-Sa 09:00"), isEmpty()); + assertThat(checkOpeningHourSyntax(key, "fixme"), not(isEmpty())); + assertThat(checkOpeningHourSyntax(key, "daily"), not(isEmpty())); + assertThat(checkOpeningHourSyntax(key, "Mo-Fr 13:30,17:45,19:00; Sa 15:00; Su 11:00"), isEmpty()); + assertThat(checkOpeningHourSyntax(key, "Mo-Fr 13:30, 17:45, 19:00; Sa 15:00; Su 11:00"), hasSize(1)); assertEquals(Severity.OTHER, - openingHourTest.checkOpeningHourSyntax(key, "Mo-Fr 13:30, 17:45, 19:00; Sa 15:00; Su 11:00").get(0).getSeverity()); - assertEquals("Mo-Fr 13:30,17:45,19:00; Sa 15:00; Su 11:00", - openingHourTest.checkOpeningHourSyntax(key, "Mo-Fr 13:30, 17:45, 19:00; Sa 15:00; Su 11:00").get(0).getPrettifiedValue()); + checkOpeningHourSyntax(key, "Mo-Fr 13:30, 17:45, 19:00; Sa 15:00; Su 11:00").get(0).getSeverity()); + assertFixEquals("Mo-Fr 13:30,17:45,19:00; Sa 15:00; Su 11:00", + checkOpeningHourSyntax(key, "Mo-Fr 13:30, 17:45, 19:00; Sa 15:00; Su 11:00").get(0)); } @@ -226,6 +233,6 @@ } for (final Tag t : values) { - final List errors = openingHourTest.checkOpeningHourSyntax(t.getKey(), t.getValue()); - if (!errors.isEmpty() && errors.get(0).getMessage().startsWith("Holiday after weekday")) { + final List errors = checkOpeningHourSyntax(t.getKey(), t.getValue()); + if (!errors.isEmpty() && errors.get(0).getDescription().startsWith("Holiday after weekday")) { continue; } @@ -240,5 +247,21 @@ public void testTicket17932() { Logging.clearLastErrorAndWarnings(); - assertTrue(openingHourTest.checkOpeningHourSyntax("opening_hours", "SH off").isEmpty()); + assertTrue(checkOpeningHourSyntax("opening_hours", "SH off").isEmpty()); + } + + private List checkOpeningHourSyntax(final String key, final String value, final Locale... locales) { + final Locale locale = locales.length > 0 ? locales[0] : Locale.ENGLISH; + final Node node = new Node(LatLon.ZERO); + node.put(key, value); + new DataSet(node); + return openingHourTest.checkOpeningHourSyntax(key, value, node, locale); + } + + private static void assertFixEquals(String value, TestError error) { + assertNotNull("fix is not null", error.getFix()); + assertTrue("fix is ChangePropertyCommand", error.getFix() instanceof ChangePropertyCommand); + final ChangePropertyCommand command = (ChangePropertyCommand) error.getFix(); + assertEquals(1, command.getTags().size()); + assertEquals(value, command.getTags().values().iterator().next()); } }