Changeset 19201 in josm for trunk/src

Timestamp:

2024-08-19T23:47:08+02:00 (5 months ago)

Author:

taylor.smock

Message:

Fix #23290: Exclude incomplete relations from region checks

Also rework TagCheckerTest to use parameterized tests.

File:

• trunk/src/org/openstreetmap/josm/data/validation/tests/TagChecker.java (modified) (12 diffs)

trunk/src/org/openstreetmap/josm/data/validation/tests/TagChecker.java

@@ -184 +184 @@
     private static final int MAX_LEVENSHTEIN_DISTANCE = 2;
 
+    /* Common string values */
+    private static final String FIXME_STR = marktr("fixme");
+    private static final String MULTIPOLYGON_TAGS = marktr("Multipolygon tags");
+    private static final String SEAMARK_TYPE = "seamark:type";
+
     protected boolean includeOtherSeverity;
 
@@ -293 +298 @@
                 BufferedReader reader = cf.getContentReader()
             ) {
-                String okValue = null;
-                boolean tagcheckerfile = false;
-                boolean ignorefile = false;
-                boolean isFirstLine = true;
-                String line;
-                while ((line = reader.readLine()) != null) {
-                    if (line.isEmpty()) {
-                        // ignore
-                    } else if (line.startsWith("#")) {
-                        if (line.startsWith("# JOSM TagChecker")) {
-                            tagcheckerfile = true;
-                            Logging.error(tr("Ignoring {0}. Support was dropped", source));
-                        } else
-                        if (line.startsWith("# JOSM IgnoreTags")) {
-                            ignorefile = true;
-                            if (!DEFAULT_SOURCES.contains(source)) {
-                                Logging.info(tr("Adding {0} to ignore tags", source));
-                            }
-                        }
-                    } else if (ignorefile) {
-                        parseIgnoreFileLine(source, line);
-                    } else if (tagcheckerfile) {
-                        // ignore
-                    } else if (line.charAt(0) == '+') {
-                        okValue = line.substring(1);
-                    } else if (line.charAt(0) == '-' && okValue != null) {
-                        String hk = harmonizeKey(line.substring(1));
-                        if (!okValue.equals(hk) && harmonizedKeys.put(hk, okValue) != null && Logging.isDebugEnabled()) {
-                            Logging.debug("Line was ignored: " + line);
-                        }
-                    } else {
-                        Logging.error(tr("Invalid spellcheck line: {0}", line));
-                    }
-                    if (isFirstLine) {
-                        isFirstLine = false;
-                        if (!(tagcheckerfile || ignorefile) && !DEFAULT_SOURCES.contains(source)) {
-                            Logging.info(tr("Adding {0} to spellchecker", source));
-                        }
-                    }
-                }
+                parseSource(source, reader);
             } catch (IOException e) {
                 Logging.error(e);
@@ -343 +309 @@
                     "Could not access data file:\n{0}",
                     "Could not access data files:\n{0}", errorSources.length(), errorSources));
+    }
+
+    private static void parseSource(String source, BufferedReader reader) throws IOException {
+        String okValue = null;
+        boolean tagcheckerfile = false;
+        boolean ignorefile = false;
+        boolean isFirstLine = true;
+        String line;
+        while ((line = reader.readLine()) != null) {
+            if (line.isEmpty()) {
+                // ignore
+            } else if (line.startsWith("#")) {
+                if (line.startsWith("# JOSM TagChecker")) {
+                    tagcheckerfile = true;
+                    Logging.error(tr("Ignoring {0}. Support was dropped", source));
+                } else
+                if (line.startsWith("# JOSM IgnoreTags")) {
+                    ignorefile = true;
+                    if (!DEFAULT_SOURCES.contains(source)) {
+                        Logging.info(tr("Adding {0} to ignore tags", source));
+                    }
+                }
+            } else if (ignorefile) {
+                parseIgnoreFileLine(source, line);
+            } else if (tagcheckerfile) {
+                // ignore
+            } else if (line.charAt(0) == '+') {
+                okValue = line.substring(1);
+            } else if (line.charAt(0) == '-' && okValue != null) {
+                String hk = harmonizeKey(line.substring(1));
+                if (!okValue.equals(hk) && harmonizedKeys.put(hk, okValue) != null && Logging.isDebugEnabled()) {
+                    Logging.debug("Line was ignored: " + line);
+                }
+            } else {
+                Logging.error(tr("Invalid spellcheck line: {0}", line));
+            }
+            if (isFirstLine) {
+                isFirstLine = false;
+                if (!(tagcheckerfile || ignorefile) && !DEFAULT_SOURCES.contains(source)) {
+                    Logging.info(tr("Adding {0} to spellchecker", source));
+                }
+            }
+        }
     }
 
@@ -435 +444 @@
         additionalPresetsValueData.addAll(Config.getPref().getList(
                 ValidatorPrefHelper.PREFIX + ".knownkeys",
-                Arrays.asList("is_in", "int_ref", "fixme", "population")));
+                Arrays.asList("is_in", "int_ref", FIXME_STR, "population")));
     }
 
@@ -584 +593 @@
      * @deprecated since 18281 -- use {@link TaggingPresets#isKeyInPresets(String)} instead
      */
-    @Deprecated
+    @Deprecated(since = "18281", forRemoval = true)
     public static boolean isKeyInPresets(String key) {
         return TaggingPresets.isKeyInPresets(key);
@@ -667 +676 @@
             if (checkFixmes && key != null && !Utils.isEmpty(value) && isFixme(key, value) && !withErrors.contains(p, "FIXME")) {
                 errors.add(TestError.builder(this, Severity.OTHER, FIXME)
-                        .message(tr("fixme"))
+                        .message(tr(FIXME_STR))
                         .primitives(p)
                         .build());
@@ -867 +876 @@
             return ((IWay<?>) primitive).getNodes().stream().anyMatch(n -> primitiveInRegions(n, regions, excludeRegions));
         } else if (primitive instanceof IRelation) {
-            return ((IRelation<?>) primitive).getMemberPrimitivesList().stream().anyMatch(p -> primitiveInRegions(p, regions, excludeRegions));
+            final IRelation<?> relation = (IRelation<?>) primitive;
+            if (relation.hasIncompleteMembers()) {
+                return true; // Assume that the relation has members in a valid area. See #23290.
+            }
+            return relation.getMemberPrimitivesList().stream().anyMatch(p -> primitiveInRegions(p, regions, excludeRegions));
         }
         throw new IllegalArgumentException("Unknown primitive type: " + primitive);
@@ -904 +917 @@
             // accept often used tag surface=* as area tag
             builder = TestError.builder(this, Severity.OTHER, MULTIPOLYGON_INCOMPLETE)
-                    .message(tr("Multipolygon tags"), marktr("only {0} tag"), "surface");
+                    .message(tr(MULTIPOLYGON_TAGS), marktr("only {0} tag"), "surface");
         } else {
             Map<String, String> filteredTags = p.getInterestingTags();
@@ -913 +926 @@
             if (filteredTags.isEmpty()) {
                 builder = TestError.builder(this, Severity.ERROR, MULTIPOLYGON_NO_AREA)
-                        .message(tr("Multipolygon tags"), marktr("tag describing the area is missing"), new Object());
+                        .message(tr(MULTIPOLYGON_TAGS), marktr("tag describing the area is missing"), new Object());
 
             }
@@ -920 +933 @@
             // multipolygon has either no area tag or a rarely used one
             builder = TestError.builder(this, Severity.WARNING, MULTIPOLYGON_MAYBE_NO_AREA)
-                    .message(tr("Multipolygon tags"), marktr("tag describing the area might be missing"), new Object());
+                    .message(tr(MULTIPOLYGON_TAGS), marktr("tag describing the area might be missing"), new Object());
         }
         errors.add(builder.primitives(p).build());
@@ -984 +997 @@
                 || p.hasTag("indoor", "corridor", "room", "area")
                 || p.hasTag("power", "substation", "generator", "plant", "switchgear", "converter", "sub_station")
-                || p.hasTag("seamark:type", "harbour", "fairway", "anchorage", "landmark", "berth", "harbour_basin",
+                || p.hasTag(SEAMARK_TYPE, "harbour", "fairway", "anchorage", "landmark", "berth", "harbour_basin",
                         "separation_zone")
-                || (p.get("seamark:type") != null && p.get("seamark:type").matches(".*\\_(area|zone)$")))
+                || (p.get(SEAMARK_TYPE) != null && p.get(SEAMARK_TYPE).matches(".*\\_(area|zone)$")))
             return true;
         return p.hasTag("harbour", OsmUtils.TRUE_VALUE)
@@ -1259 +1272 @@
 
     private static boolean isFixme(String key, String value) {
-        return key.toLowerCase(Locale.ENGLISH).contains("fixme") || key.contains("todo")
-          || value.toLowerCase(Locale.ENGLISH).contains("fixme") || value.contains("check and delete");
+        return key.toLowerCase(Locale.ENGLISH).contains(FIXME_STR) || key.contains("todo")
+          || value.toLowerCase(Locale.ENGLISH).contains(FIXME_STR) || value.contains("check and delete");
     }