Modify

Opened 13 months ago

Closed 12 months ago

Last modified 11 months ago

#20902 closed enhancement (fixed)

[Patch] Warn about solo layer and level tags

Reported by: skyper Owned by: Klumbumbus
Priority: normal Milestone: 21.06
Component: Core validator Version:
Keywords: template_report only tag Cc: Klumbumbus

Description

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

Attachments (6)

josm_combinations_only_tag.patch (1.9 KB) - added by skyper 13 months ago.
patch
josm_combinations_only_tag_v2.patch (2.2 KB) - added by skyper 13 months ago.
version 2: do not warn about some type of relations with layer plus exclude multipolygon as there are other warnings
josm_combinations_only_tag_v3.patch (2.3 KB) - added by skyper 13 months ago.
version 3: additionally, exclude building, bridge and tunnel relations from "name" test
josm_combinations_only_tag_v4.patch (3.0 KB) - added by skyper 13 months ago.
version 4: tested with all relation types in defaultpreset
josm_20902_v5.patch (2.0 KB) - added by skyper 12 months ago.
only test nodes and ways
josm_20902_v6.patch (3.9 KB) - added by skyper 12 months ago.
version 6: do not check almost all relation in combinations.mapcss and better handling for relations in unnecessary.mapcss

Download all attachments as: .zip

Change History (29)

Changed 13 months ago by skyper

patch

Changed 13 months ago by skyper

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

comment:1 Changed 13 months ago by 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.

comment:2 Changed 13 months ago by 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

Last edited 13 months ago by skyper (previous) (diff)

Changed 13 months ago by skyper

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

comment:3 Changed 13 months ago by 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?

comment:4 Changed 13 months ago by 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.

comment:5 in reply to:  4 ; Changed 13 months ago by 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.

Changed 13 months ago by skyper

version 4: tested with all relation types in defaultpreset

comment:6 in reply to:  5 Changed 13 months ago by 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.

Example: *[/^lanes|access$/].one_tag[type !~ /^(boundary|connectivity|enforcement|multipolygon|restriction|route|waterway)$/]

Last edited 13 months ago by reichg (previous) (diff)

comment:7 Changed 13 months ago by 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!

comment:8 in reply to:  7 Changed 12 months ago by 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?

comment:9 Changed 12 months ago by skyper

Cc: Klumbumbus added

comment:10 Changed 12 months ago by skyper

Milestone: 21.06

comment:11 Changed 12 months ago by 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?

comment:12 Changed 12 months ago by skyper

Owner: changed from team to 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.

comment:13 Changed 12 months ago by skyper

Milestone: 21.0621.07

Changed 12 months ago by skyper

Attachment: josm_20902_v5.patch added

only test nodes and ways

comment:14 Changed 12 months ago by skyper

Milestone: 21.0721.06
Owner: changed from skyper to Klumbumbus

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.

comment:15 Changed 12 months ago by Klumbumbus

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

In which case nodes are used with level only?

comment:16 Changed 12 months ago by 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.

Last edited 12 months ago by skyper (previous) (diff)

comment:17 Changed 12 months ago by 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");
}

comment:18 in reply to:  17 Changed 12 months ago by 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.

comment:19 Changed 12 months ago by skyper

Owner: changed from Klumbumbus to 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.

Changed 12 months ago by skyper

Attachment: josm_20902_v6.patch added

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

comment:20 Changed 12 months ago by skyper

Owner: changed from skyper to Klumbumbus

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.

comment:21 Changed 12 months ago by Klumbumbus

Resolution: fixed
Status: newclosed

In 17957/josm:

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

comment:22 Changed 12 months ago by 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]

comment:23 Changed 11 months ago by skyper

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Klumbumbus.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.