Modify

Opened 11 months ago

Closed 9 months ago

#7811 closed defect (fixed)

[Patch] Vague "Suspicious tag/value combinations" message for absent denomination tag

Reported by: mrwojo Owned by: team
Priority: normal Component: Core validator
Version: latest Keywords: Suspicious tag value combinations TagChecker
Cc:

Description

When something is tagged religion = (christian | muslim | jewish | pastafarian | other), the TagChecker validator expects a denomination tag. If that tag is absent or has an unexpected value, TagChecker emits the vague info-level message "Suspicious tag/value combinations" (warning 1272).

I had to consult TagChecker.java to figure out what tags that message was referencing -- it's the T: lines in ignoretags.cfg.

The history of TagChecker.java and ignoretags.cfg reveals that I'm not the first struck by warning 1272. The attached patch removes 1272. ignoretags.cfg should only cause warnings to be ignored, it shouldn't produce its own.

Attachments (2)

tagchecker2.patch (2.2 KB) - added by mrwojo 11 months ago.
tagchecker2.2.patch (10.6 KB) - added by mrwojo 9 months ago.

Download all attachments as: .zip

Change History (14)

Changed 11 months ago by mrwojo

comment:1 follow-up: Changed 11 months ago by simon04

  • Resolution set to invalid
  • Status changed from new to closed

Please note that this validation "error" is of type informational level. This level has to be activated manually and indicates that there might be some/many false-positives. I don't see a necessity to change this.

comment:2 in reply to: ↑ 1 Changed 11 months ago by skyper

Replying to simon04:

Please note that this validation "error" is of type informational level. This level has to be activated manually and indicates that there might be some/many false-positives. I don't see a necessity to change this.

Sorry, I do not get this.

Why does validator not just tell what is considered wrong (e.g. missing denomination tag). Then this could even be a "warning" and not just an info.

comment:3 follow-ups: Changed 11 months ago by mrwojo

This level has to be activated manually and indicates that there might be some/many false-positives.

The actual problem here though is that the message is useless unless you dig through JOSM's source.

Why does validator not just tell what is considered wrong (e.g. missing denomination tag).

We can do this -- the correct way -- easily in tagchecker.cfg, just like with "restaurant without name" messages.

comment:4 Changed 11 months ago by simon04

  • Resolution invalid deleted
  • Status changed from closed to reopened

I agree that there is room for improvements (e.g., meaningful message).

comment:5 in reply to: ↑ 3 Changed 11 months ago by skyper

  • Keywords Suspicious tag value combinations added
  • Version set to latest

Replying to mrwojo:

Why does validator not just tell what is considered wrong (e.g. missing denomination tag).

We can do this -- the correct way -- easily in tagchecker.cfg, just like with "restaurant without name" messages.

+1

comment:6 Changed 10 months ago by simon04

That part of the code was introduced in [o16436], #2803. I do not fully understand the intention behind this code. If we drop it, then the the other parts of [o16436] and the lines starting with T: in ignoretags.cfg should be dropped as well.

comment:7 Changed 10 months ago by mrwojo

Good point. The part where it ignores tags examines key2 & value2 but not key1 & value1. Thus, T:foo=bar|baz=qux has actually functioned no differently from K:baz=qux apart from also emitting 1272.

Repro: You can see T: ignoring denomination preset warnings even without the presence of a religion tag. Create a node with only a denomination tag:

  • denomination=foo gives "Value 'foo' for key 'denomination' not in presets. - Presets do not contain property value (1)". This is expected behavior.
  • denomination=alaouite gives no warning. This is unexpected behavior. I'd expect the "not in presets" warning to be ignored only when the node also has religion=muslim (as implied by T:religion=muslim|denomination=alaouite).

We could replace the T: with K: lines (such as, K:denomination=alaouite).

comment:8 Changed 10 months ago by mrwojo

  • Keywords TagChecker added

comment:9 in reply to: ↑ 3 Changed 10 months ago by bastiK

Replying to mrwojo:

This level has to be activated manually and indicates that there might be some/many false-positives.

The actual problem here though is that the message is useless unless you dig through JOSM's source.

Why does validator not just tell what is considered wrong (e.g. missing denomination tag).

We can do this -- the correct way -- easily in tagchecker.cfg, just like with "restaurant without name" messages.

The following additions to tagchecker.cfg should result in more meaningful validator messages. In addition, when religion=* is present, but denomination=* is missing, this doesn't trigger an info-level warning any longer.

* : I : religion == christian && denomination == * && denomination != /anglican|apostolic|baptist|catholic|christian_community|christian_scientist|coptic_orthodox|czechoslovak_hussite|dutch_reformed|evangelical|foursquare|greek_orthodox|jehovahs_witness|kabbalah|karaite|living_waters_church|lutheran|maronite|mennonite|methodist|mormon|new_apostolic|nondenominational|old_catholic|orthodox|pentecostal|presbyterian|protestant|quaker|roman_catholic|russian_orthodox|salvation_army|seventh_day_adventist|united|united_reformed|uniting/  # unkown christian denomination
* : I : religion == muslim && denomination == * && denomination != /alaouite|druze|ibadi|ismaili|nondenominational|shia|sunni/ # unkown muslim denomination
* : I : religion == jewish && denomination == * && denomination != /alternative|ashkenazi|conservative|hasidic|humanistic|liberal|modern_orthodox|neo_orthodox|nondenominational|orthodox|progressive|reconstructionist|reform|renewal|samaritan|ultra_orthodox/ # unkown jewish denomination

@mrwojo: Can you make a patch that adds it all up?

Changed 9 months ago by mrwojo

comment:10 Changed 9 months ago by mrwojo

Sorry, I was under the impression that we wanted an info-level warning for missing denomination. My original patch simply removed that warning.

The new patch leaves it to the normal handling to check for denomination values in presets and adds a clear tagchecker message ("religion without denomination") when you've got a religion=christian|jewish|muslim tag without a denomination tag.

comment:11 Changed 9 months ago by stoecker

@bastiK: What is the status of this?

comment:12 Changed 9 months ago by bastiK

  • Resolution set to fixed
  • Status changed from reopened to closed

In 5475/josm:

applied #7811 - Vague "Suspicious tag/value combinations" message for absent denomination tag (patch by mrwojo, slightly modified)

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed .
as The resolution will be set. Next status will be 'closed'.
The resolution will be deleted. Next status will be 'reopened'.
Author


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

 
Note: See TracTickets for help on using tickets.