Modify

Opened 7 years ago

Closed 7 years ago

#7811 closed defect (fixed)

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

Reported by: mrwojo Owned by: team
Priority: normal Milestone:
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 7 years ago.
tagchecker2.2.patch (10.6 KB) - added by mrwojo 7 years ago.

Download all attachments as: .zip

Change History (14)

Changed 7 years ago by mrwojo

Attachment: tagchecker2.patch added

comment:1 Changed 7 years ago by simon04

Resolution: invalid
Status: newclosed

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 7 years 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 Changed 7 years 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 7 years ago by simon04

Resolution: invalid
Status: closedreopened

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

comment:5 in reply to:  3 Changed 7 years ago by skyper

Keywords: Suspicious tag value combinations added
Version: 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 7 years 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 7 years 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 7 years ago by mrwojo

Keywords: TagChecker added

comment:9 in reply to:  3 Changed 7 years 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 7 years ago by mrwojo

Attachment: tagchecker2.2.patch added

comment:10 Changed 7 years 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 7 years ago by stoecker

@bastiK: What is the status of this?

comment:12 Changed 7 years ago by bastiK

Resolution: fixed
Status: reopenedclosed

In 5475/josm:

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
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.