false positives from tagchecker with single letter differences

[GerdP]

Follow up for #17055

I've done the steps described in ticket:17055#comment:55 again on updated taginfo data to get this list:

Value 'y' for key 'x' is unknown, maybe 'z' is meant?

tag	know value	tagcount	decision
access=customer	customers	1024|102|910|12	✔️ added to deprecated.mapcss and ignorelist
addr:inclusion=estimated	estimate	1301|0|1301|0	✔️ added to deprecated.mapcss and ignorelist
building=apartment	apartments	2368|84|2284|0	✔️ added to deprecated.mapcss and ignorelist
building=constructie	construction	3949|0|3949|0	✖️ it's a right positive (even if these buildings are not under construction for real) and it appears only on one spot ​overpass turbo
building=home	hotel, house	1462|706|756|0	✔️ added to ignorelist
building=part	barn, farm	1192|322|828|42	✔️ added to deprecated.mapcss and ignorelist
building=ruin	ruins	1680|8|1672|0	✖️ it's a right positive
building=tank	bank	2902|11|2890|1	✔️ added to ignorelist
building=yes,	yes	2136|0|2136|0	✔️ only in two villages, fixed in the osm database
generator:type=solar_photovoltaic_panels	solar_photovoltaic_panel	6702|8|6679|15	✔️ added to deprecated.mapcss and ignorelist
historic=farm	fort	2125|316|1632|177	✔️ added to ignorelist
lamp_mount=bent mast	bent_mast	10347|10334|13|0	✔️ added to deprecated.mapcss and ignorelist
lamp_mount=straight mast	straight_mast	13124|13108|16|0	✔️ added to deprecated.mapcss and ignorelist
lamp_type=electrical	electric	3678|3678|0|0	✔️ added to deprecated.mapcss and ignorelist
lanes=-1	1	1820|0|1820|0	✔️ already in numerical.mapcss, added to ignorelist
man_made=water_tank	water_tap	4243|2433|1807|3	✔️ already in deprecated.mapcss, added to ignorelist
resource=clay	coal	1111|2|1088|21	✔️ added to presets
sport=skiing	skating	5857|1157|4502|198	✔️ already in deprecated.mapcss, added to ignorelist
toilets:disposal=pit_latrine	pitlatrine	3993|3993|0|0	✔️ was already fixed in the osm database (​taghistory)
water=pool	pond	2609|0|2583|26	✔️ added to ignorelist

I think these tags should be treated in presets or ignoretags.cfg:
historic=farm
resource=clay
sport=skiing

Maybe add a mapcss check for these which proposes a different tag?
building=part -> building:part=yes
building=tank -> man_made=storage_tank or water_tank or ?
water=pool -> amenity=swimming_pool

The building='yes,' objects are from hotosm projects, I've commented some CS.

[Klumbumbus]

(convert into table)

[Klumbumbus]

(accidently commited 3 rules in r16752)

[Klumbumbus]

Hm, I thought if a tag has an explizit deprecation warning in deprecated.mapcss then it would be removed from the "Value 'y' for key 'x' is unknown, maybe 'z' is meant" warning, but it is not, so we need to add it to the ignore list too.

[GerdP]

Replying to Klumbumbus:

Hm, I thought if a tag has an explizit deprecation warning in deprecated.mapcss then it would be removed from the "Value 'y' for key 'x' is unknown, maybe 'z' is meant" warning

I think the validator rules in *.mapcss are ignored. No idea if it would be possible to use them, at least it would require new code.
TagChecker learns from presets, words.cfg and ignoretags.cfg. So if it suggest a deprecated value these sources should be checked.

[Klumbumbus]

I always thought if a deprecation warning exists it was automatically removed from the "Value y for key x not in presets" info-warning, but it seems I'm wrong here too. Well it is a very minor problem, only duplicate warnings in severity warning and other.

[Klumbumbus]

In 16760/josm:

see #19180 - deprecate access=customer, addr:inclusion=estimated, building=apartment, lamp_mount="bent mast", lamp_mount="straight mast", lamp_type=electrical (common typos) and add some tags to ignore list

[Klumbumbus]

In 16762/josm:

see #19180 - Add most tags from deprecated.mapcss to ignorelist to clean up a bit the severity other group of validator results

[skyper]

Replying to Klumbumbus:

In 16762/josm:

see #19180 - Add most tags from deprecated.mapcss to ignorelist to clean up a bit the severity other group of validator results

Mmh, did you check how this works if you have some tests/groups one the ignore list or even disabled in preferences. I always thought it is a good idea to only have one warning for one issue but lately, I started to change my mind and rather have duplicates instead of missing some cases.

[GerdP]

Ah, come on, if you like duplicates you don't put something into the ignore list.

[Klumbumbus]

Yes, also the other way round, if someone disables tests or puts them on ignore list it is his intention to not get a warning so he probably does not like the info-warning too. Also it is clear if you disable/ignore tests you don't get the "full validator support".

[Klumbumbus]

But we have another problem I was not aware before. The ignorelist contains good and bad tags/keys. And the test "Value 'y' for key 'x' is unknown, maybe 'z' is meant?" takes all entries from the ignore list as good values. So if you tag highway=fard the validator says Value 'fard' for key 'highway' is unknown, maybe 'ford' is meant?" but highway=ford is deprecated.
r16762 made this problem even worse.

Could we remove ignoretag.cfg as dictionary for good values or what was the reason for that?

[GerdP]

see ticket:17055#comment:40

[GerdP]

ignoretags.cfg should contain those tags which are often used but not contained in presets. Maybe we should implement a new key D: for deprecated tags so that TagChecker can show a different message like
Value 'y' for key 'x' is unknown, maybe deprecated 'z' is meant?

[Klumbumbus]

Replying to GerdP:

a new key D: for deprecated tags

That sounds like a solution.

so that TagChecker can show a different message like
Value 'y' for key 'x' is unknown, maybe deprecated 'z' is meant?

I think the valdator should not suggest a deprecated value. Better would be in this case if tagchecker falls back to the message "Value y for key x not in presets".

Value fard for key highway not in presets
is better than
Value 'fard' for key 'highway' is unknown, maybe deprecated 'ford' is meant?

[GerdP]

OK, I'll try to code this.

[Klumbumbus]

Replying to Klumbumbus:

Replying to GerdP:

a new key D: for deprecated tags

That sounds like a solution.

Ah, no "bad" entries need the distinction between K E S and F too. See e.g. the deprecated entries:

K:highway=ford
E:power_source
S:color:
F::color

[Klumbumbus]

We could define 4 new letters or create an own file for the deprecated entries?

[GerdP]

Looking at the rules in deprecated.mapcss there are also cases where a tag is only deprecated when it is not used with another tag.

*[place_name][!name] {
  throwWarning: tr("{0} should be replaced with {1}", "{0.key}", "{1.key}");
  fixChangeKey: "place_name => name";
}

We cannot code that with the simple format used in igoretags.cfg. Maybe I can change the code so that TagChecker can call its own instance of MapCssTagChecker to verify that the suggested tag(s) is(are) not flagged as deprecated.

[Klumbumbus]

Replying to GerdP:

Looking at the rules in deprecated.mapcss there are also cases where a tag is only deprecated when it is not used with another tag.

Yes I noticed too that this requires some "human thinking". Some rules are split (e.g. with and without autofix) or not for all object types. Some rules use e.g. node as object selector because the tag is only deprecated on nodes and some because it appears only on nodes (* was not used as selector there for performance reasons).
Thats why I added the hand-picked list in r16762.

Maybe I can change the code so that TagChecker can call its own instance of MapCssTagChecker to verify that the suggested tag(s) is(are) not flagged as deprecated.

If that would be possible that would be good. Else I think we can live with a hand-picked list?

[GerdP]

BTW: Your latest changes contain some typos:

2020-07-14 16:40:23.123 SEVERE: Invalid line in resource://data/validator/ignoretags.cfg : 'generator:location' does not contain '='
2020-07-14 16:40:23.124 SEVERE: Invalid line in resource://data/validator/ignoretags.cfg : 'monitoring:river_level' does not contain '='
2020-07-14 16:40:23.124 SEVERE: Invalid line in resource://data/validator/ignoretags.cfg : 'fire_hydrant:flow_capacity' does not contain '='
2020-07-14 16:40:23.124 SEVERE: Invalid line in resource://data/validator/ignoretags.cfg : 'fire_hydrant:water_source' does not contain '='
2020-07-14 16:40:23.125 SEVERE: Invalid line in resource://data/validator/ignoretags.cfg : 'is_in' does not contain '='
2020-07-14 16:40:23.125 SEVERE: Invalid line in resource://data/validator/ignoretags.cfg : 'building:type' does not contain '='
2020-07-14 16:40:23.125 SEVERE: Invalid line in resource://data/validator/ignoretags.cfg : 'recycling:metal' does not contain '='

[Klumbumbus]

In 16763/josm:

see #19180 - fix typos in ignorelist, remove police=yes from ignorelist (has warning but is also in presets for an unknown police facility), remove deprecated vending=photos from presets

[GerdP]

The patch is not complete yet, it only suppresses the "Value 'y' for key 'x' is unknown, maybe 'z' is meant?" messages.
I'll do the same for "Key 'x' looks like 'y'."

[GerdP]

handle also the "Key 'x' looks like 'y' " message

[GerdP]

• For highway=fard we now get Info - Presets do not contain property value - Value 'fard' for key 'highway' not in presets.
• For hihgway=ford we now get Info - Presets do not contain property key - Key 'hihgway' not in presets. instead of Warning - Misspelled property key - Key 'hihgway' looks like 'highway'.
• For hihgway=stop we get still get Warning Misspelled property key - Key 'hihgway' looks like 'highway'.

I think I prefer the 1st patch which would produce the warning also for hihgway=ford

[Klumbumbus]

Replying to GerdP:

I think I prefer the 1st patch which would produce the warning also for hihgway=ford

Yes that's OK as it doesn't directly suggest a deprecated tag.

[GerdP]

Wait a minute. Why do we have highway=ford in ignoredtags.cfg when we also have a test in deprecated.mapcss?
I think my patch is completely unnecessary, we just have to remove the entries from ignoredtags.cfg.

[Klumbumbus]

Replying to GerdP:

Why do we have highway=ford in ignoredtags.cfg when we also have a test in deprecated.mapcss?

To suppress the unnecessary info-warning "value ford for key highway not in presets".

[GerdP]

Well, with my patch and the entry in ignoretags.cfg we still have this warning. I coded it like that because of your suggestion
in comment:22

Better would be in this case if tagchecker falls back to the message "Value y for key x not in presets".

So, best solution would be to produce no message for a tag which is flagged as deprecated, right?
Means, we have to change the logic of the patch and we can remove deprecated tags from ignoretags.cfg

[GerdP]

No, sorry, I should not make assumptions that late in the night. Tested again the 1st patch applied on r16780 with all 4 variants:

• highway=ford gives Warning - deprecated tagging - highway=ford is deprecated, use ford=yes instead
• highway=fard gives Other - Presets do not contain property value - Value 'fard' for key 'highway' not in presets.
• hihgway=ford gives Warning - Misspelled property key - Key 'hihgway' looks like 'highway'.
• hihgway=fard also gives Warning - Misspelled property key - Key 'hihgway' looks like 'highway'.

Without the patch:

• highway=ford same
• highway=fard gives wrong Warning - Unknown property value - Value 'fard' for key 'highway' is unknown, maybe 'ford' is meant?
• hihgway=ford same
• hihgway=fard same

Without highway=ford in ignoredtags.cfg we see two warnings for highway=ford

• OK: deprecated tagging - highway=ford is deprecated, use ford=yes instead
• KO: Unknown property value - Value 'ford' for key 'highway' is unknown, maybe 'road' is meant?

The other variants are as with the patch.

So, yes, the 1st patch seems to be the best solution so far. Commit it?

[Klumbumbus]

I just noticed, "hihgway" might not be the best example mispelled key for testing as it is in words.cfg too.

other example which is not in words.cfg:

with patch v1 highwoy=ford gives Other - Presets do not contain property key - Key 'highwoy' not in presets. which is fine.

So I think patch v1 is fine to commit.

[GerdP]

In 16784/josm:

see #19180: false positives from tagchecker with single letter differences

• avoid to produce "Value 'y' for key 'x' is unknown, maybe 'z' is meant?" when x=z produces a deprecated message. It uses all *.mapcss rules which are stored in files ending with deprecated.mapcss, so this will fail if user changed the name to something else, but it should work with e.g. my_deprecated.mapcss

[skyper]

Replying to GerdP:

In 16784/josm:

see #19180: false positives from tagchecker with single letter differences

• avoid to produce "Value 'y' for key 'x' is unknown, maybe 'z' is meant?" when x=z produces a deprecated message. It uses all *.mapcss rules which are stored in files ending with deprecated.mapcss, so this will fail if user changed the name to something else, but it should work with e.g. my_deprecated.mapcss

The correct ending of a local validator file is .validator.mapcss. At least, that is the ending in the file manager when adding a new rule.

[GerdP]

I mentioned this because for a developer it is comfortable to configure the path to the local svn repo. This means that a change to the file in the repo triggers the reload of the rules.
If one changes the local file name to something like deprecated_experimental.mapcss it may cause confusion.

[Klumbumbus]

In 16787/josm:

see #19180

• deprecate generator:type=solar_photovoltaic_panels and building=part
• ignore building=home, building=tank, water=pool, historic=farm
• add resource=clay

• adjust fixme icons to avoid possible rendering artifacts

[GerdP]

I've just noticed that r16784 introduced a performance problem when data contains some ways with e.g. natural=shoal.
I work on a fix which requires no changes in i18n strings. Is it OK to commit that before new JOSM is released?

[GerdP]

performance improvement

[GerdP]

The patch

• avoids to modify the original object as this requires a writeLock and probably causes trouble with locked files
• avoids to call the rather complex filterDeprecatedTags() method by checking LEVENSHTEIN_DISTANCE first

[GerdP]

Committed with r16812, see #19598.

[simon04]

In 17621/josm:

see #19180 - Fix code duplication