[Patch] Warn about solo layer and level tags

[skyper]

What steps will reproduce the problem?

1. Have an object with only layer=*
2. Run validator

What is the expected result?

A warning about missing tags

What happens instead?

No warning except for relations

Please provide any additional information below. Attach a screenshot if possible.

Please, find attached patch​ which adds layer and for ways and relations level. I am not sure about node and a solo level=*.
I introduced a class to warn about relations with type=* and one more tag, as there is already a warning about relation without type.

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2021-05-17 21:27:21 +0200 (Mon, 17 May 2021)
Revision:17903
Build-Date:2021-05-18 01:31:02
URL:https://josm.openstreetmap.de/svn/trunk

[skyper]

patch

[skyper]

version 2: do not warn about some type of relations with layer plus exclude multipolygon as there are other warnings

[skyper]

josm_combinations_only_tag_v2.patch​ excludes type=bridge|building|tunnel from layer and additionally does not warn about multipolygons except of surface as there are already warnings on same or higher warning level from java TagChecker test. See also #20909.

[skyper]

Damn, I need to exclude some more relations from name, too, as type=bridge|building|tunnel are valid. See josm_combinations_only_tag_v3.patch​

[skyper]

version 3: additionally, exclude building, bridge and tunnel relations from "name" test

[mdk]

Did I understand the syntax correctly? Does 0.tag means the tag name of the first tag? What is about relations, where you set one_tag if there only one other tag besides type? Did 0.tag always match the other tag or could we get (confusing) errors like incomplete object: only type when the other key is the second one?

[skyper]

No, 0.tag is the tag of the first selector, see Help/Validator/MapCSSTagChecker, which is always the lonely tag as the object type does not count.

I do not change anything regarding the message. I only add two more keys to check and tried to make test more effective with relations and at the same time to hide some duplicate warnings.

Hopefully, it will not introduce new duplicate warnings but I think I have forgotten to check some relations, explicitly. Have to recheck at least boundary, route and restriction.

[skyper]

Replying to skyper:

Hopefully, it will not introduce new duplicate warnings but I think I have forgotten to check some relations, explicitly. Have to recheck at least boundary, route and restriction.

I have checked on all relation types of defaultpresets and this is the outcome: josm_combinations_only_tag_v4.patch​
Quite some relation types have special tests for needed subkeys and maybe we could also think about excluding relations in general and create better test for each type.

[skyper]

version 4: tested with all relation types in defaultpreset

[reichg]

Replying to skyper:

Replying to skyper:

Hopefully, it will not introduce new duplicate warnings but I think I have forgotten to check some relations, explicitly. Have to recheck at least boundary, route and restriction.

I have checked on all relation types of defaultpresets and this is the outcome: josm_combinations_only_tag_v4.patch​
Quite some relation types have special tests for needed subkeys and maybe we could also think about excluding relations in general and create better test for each type.

Can try to group keys together with regex like I used in 9819 patch, specifically with highway/waterway/railway. It might be possible to clean this up.

[skyper]

I have sometimes problems with too many regex in one line which often makes it harder to be more specific on certain combinations and I remember I had problems with placeholders in the warning in the past. Readability is another factor. I could extract the most common exceptions of type=*into a class though.

Anyway, I still think this test on relations could be dropped. Atm, it is completely useless and my try to make it a little useful within this test is not a good aproach. In my eyes, own test for known relations' types would work much better and some of these tags could be warned about in general with some relation types. New ticket!

[skyper]

Replying to skyper:

Anyway, I still think this test on relations could be dropped. Atm, it is completely useless and my try to make it a little useful within this test is not a good aproach. In my eyes, own test for known relations' types would work much better and some of these tags could be warned about in general with some relation types. New ticket!

Opinions?

[Klumbumbus]

I guess performance wise it is worse to check the number of tags for all objects instead of only the ones with the specific tag. Whats left from the patch when the change with the one_tag class is removed?

[skyper]

I take another look. At least the relation problem needs a fix as atm we already have a warning for relations without type=* (and many other depending on the type). So, only one tag does not work with relations.

[skyper]

only test nodes and ways

[skyper]

I simply removed relations from these rules as they better get there own tests (if they do not exist already).
See josm_20902_v5.patch​.

[Klumbumbus]

way[level][eval(number_of_tags()) = 1], /* nodes might be valid */

In which case nodes are used with level only?

[skyper]

I find nodes with only level in indoor tagging take a look at complex train stations, especially, staircases and ramps. But maybe that was invented by Mentz.

[Klumbumbus]

OK. And is there a reason you didn't change these rules too?

/* only {0.key} and {1.key} */
*[name][area][eval(number_of_tags()) = 2],
*[name][ref][eval(number_of_tags()) = 2] {
  throwWarning: tr("incomplete object: only {0} and {1}", "{0.key}", "{1.key}");
  group: tr("missing tag");
}

/* only {0.key} and {1.tag} */
*[name][tourism=attraction][eval(number_of_tags()) = 2] {
  throwWarning: tr("incomplete object: only {0} and {1}", "{0.key}", "{1.tag}");
  group: tr("missing tag");
}

[skyper]

Replying to Klumbumbus:

OK. And is there a reason you didn't change these rules too?

Nice catch. No, I just missed them as it is not only one tag. area does not work with relations. ref and tourism=attraction probably need further treatment, I guess.

[skyper]

I take another look including testing to see, what we can do with these cases with two tags and relations.
Think we should warn about area=* on any relation and offer to remove it.

[skyper]

version 6: do not check almost all relation in combinations.mapcss and better handling for relations in unnecessary.mapcss

[skyper]

josm_20902_v6.patch​ removes relations completely from the rules in combinations.mapcss (except of one case) and warns in general about area=* with relations in unnecessary.mapcss.

I think relations are too special and better have specific warnings per type. This should be handled in separate tickets. Removing them for now, as, atm, the rules do not work with relations anyway and most of them already have their own warnings.

[Klumbumbus]

In 17957/josm:

fix #20902 - Warn about solo layer and level tags, remove relations from checks, warn about area on relations (patch by skyper)

[Klumbumbus]

I adjusted/moved the general rule for relations and removed the following line as there is currently no relation in the database matching.

relation[name][tourism=attraction][eval(number_of_tags()) = 3][type=site]

[skyper]

I've created #21129 about relations without primary key.