Modify

Opened 3 months ago

Closed 2 months ago

Last modified 2 months ago

#17567 closed enhancement (fixed)

rephrase warning for role location_hint in restriction relation

Reported by: GerdP Owned by: team
Priority: normal Milestone: 19.04
Component: Core Version:
Keywords: template_report Cc: Klumbumbus

Description

What steps will reproduce the problem?

  1. Validate https://www.openstreetmap.org/relation/228078

What is the expected result?

No warning

What happens instead?

two warnings for the node with role location_hint:
Role verification problem - Role 'location_hint' unknown in templates 'from/via/to' (1)
Unknown role(1)

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

The 2nd warning comes from TurnrestrictionTest. I'll add a correction to the patch for #17561.

Build-Date:2019-04-06 10:28:00
Revision:14963
Is-Local-Build:true

Identification: JOSM/1.5 (14963 SVN en) Windows 10 64-Bit
OS Build number: Windows 10 Home 1803 (17134)
Memory Usage: 687 MB / 1753 MB (119 MB allocated, but free)
Java version: 1.8.0_191-b12, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Screen: \Display0 1920x1080
Maximum Screen Size: 1920x1080
VM arguments: [-agentlib:jdwp=transport=dt_socket,suspend=y,address=localhost:51784, -Dfile.encoding=UTF-8]
Program arguments: [--debug]
Dataset consistency test: No problems found

Plugins:
+ FastDraw (34949)
+ OpeningHoursEditor (34867)
+ apache-commons (34506)
+ buildings_tools (34904)
+ continuosDownload (82)
+ ejml (34389)
+ geotools (34513)
+ jaxb (34678)
+ jts (34524)
+ o5m (34867)
+ opendata (34911)
+ pbf (34867)
+ poly (34867)
+ reltoolbox (34867)
+ reverter (34961)
+ undelete (34919)
+ utilsplugin2 (34932)

Last errors/warnings:
- W: No configuration settings found.  Using hardcoded default values for all pools.
- W: Region [TMS_BLOCK_v2] Resetting cache

Attachments (4)

17567.patch (800 bytes) - added by GerdP 3 months ago.
17567-v2.patch (9.5 KB) - added by GerdP 2 months ago.
Proposed changes
17567.PNG (14.8 KB) - added by Klumbumbus 2 months ago.
17567-rephrase.patch (3.3 KB) - added by GerdP 2 months ago.

Download all attachments as: .zip

Change History (34)

Changed 3 months ago by GerdP

Attachment: 17567.patch added

comment:1 Changed 3 months ago by GerdP

Cc: Klumbumbus added
Summary: support role location_hint in turn restrictions[Patch] support role location_hint in turn restrictions
Type: defectenhancement

The attached patch seems to work fine. Is it OK?

comment:2 Changed 3 months ago by Klumbumbus

I never used that role. I just looked at osmwiki:Relation:restriction and it says "A hint to a renderer as to where might be a good place to position a symbol indicating the restriction." That is pure tagging for the renderer which we should not promote. (And there are only 172 of these roles in the database.)

comment:3 Changed 3 months ago by GerdP

I know the tag since a long time so I guess we should not produce any warning, esp. none with a text like "unknown".
Maybe I can modify the test in RelationChecker.

comment:4 Changed 3 months ago by Klumbumbus

We could rephrase "Role 'abc' unknown in templates 'xyz'" to "Role 'abc' not in templates 'xyz'" or similar but the warning should stay.

comment:5 Changed 3 months ago by GerdP

The rephrase is a good idea.
I've just uploaded a new patch for #17561 which would just suppress the message. I'll think again about it...

comment:6 Changed 3 months ago by GerdP

Summary: [Patch] support role location_hint in turn restrictionsrephrase warning for role location_hint in restriction relation

comment:7 Changed 3 months ago by GerdP

Resolution: fixed
Status: newclosed

In 14966/josm:

fix #17561 Confusing error message for turn restriction
fix #17567 rephrase warning for role location_hint in restriction relation

  • improve validator messages for restriction relations
  • suppress duplicate warnings for wrong roles from RelationChecker when TurnrestrictionTest is enabled
  • add unit test for TurnrestrictionTest similar to the one for MultipolygonTest

comment:8 Changed 2 months ago by Don-vip

Milestone: 19.04

comment:9 Changed 2 months ago by Klumbumbus

Hm, I thought you would reword the massage for all "unknown roles", not just for that one role location_hint, at least that makes more sense to me.

And probably we should reword "templates" to "presets".

comment:11 in reply to:  9 Changed 2 months ago by GerdP

Replying to Klumbumbus:

Hm, I thought you would reword the massage for all "unknown roles", not just for that one role location_hint, at least that makes more sense to me.

I intended to do that but it seems I got it wrong.

And probably we should reword "templates" to "presets".

I agree. Should I change all places where template means preset?

comment:12 Changed 2 months ago by GerdP

Maybe we were mislead. The unit test expects e.g. Role 'level_x' is not in templates 'outline/part/ridge/edge/entrance/level_-?\\d+'".
So the word preset would be wrong here, it seems to be about a template for the role. Right?

Last edited 2 months ago by GerdP (previous) (diff)

Changed 2 months ago by GerdP

Attachment: 17567-v2.patch added

Proposed changes

comment:13 Changed 2 months ago by Klumbumbus

With "template" the table with possible roles and appropriate object types in the lower part of relation presets is meant. As this is "in the presets" too I guess we can use the word "preset" to not confuse the user and use consistend wording.

comment:14 Changed 2 months ago by GerdP

Don't think so. I replaced template by preset here:
Look at the test string Role of relation member does not match expression 'power' in template Power Route"
In my patch this is changed to
Role of relation member does not match template expression 'power' in preset Power Route

But Role 'level_x' is not in presets 'outline/part/ridge/edge/entrance/level_-?\\d+' makes no sense.

comment:15 Changed 2 months ago by Klumbumbus

Ah yes, it seems better to keep both words.

comment:16 Changed 2 months ago by GerdP

In 14990/josm:

see #17567: improve messages created by RelationChecker

  • new error code 1709 for " Type {0} of relation member with role {1} does not match accepted types {2} in preset {3}"
  • role type -> role
  • replace template by preset where appropriate
  • adapt unit test and improve speed

Changed 2 months ago by Klumbumbus

Attachment: 17567.PNG added

comment:17 Changed 2 months ago by Klumbumbus

Resolution: fixed
Status: closedreopened

There are still different texts:

comment:18 Changed 2 months ago by GerdP

Hmm, yes, both messages come from TurnRestrictionTest which doesn't know the presets or templates.
I could change both messages to "Unknown role {0} in restriction" or maybe "Unexpected role {0} in restriction".
Alternative is to remove all tests which are also implemented in RelationChecker and keep only those which detect special cases like mixed via node/way.

Thinking about it I'd prefer the 2nd solution.

comment:19 Changed 2 months ago by GerdP

.. but we would not produce any message when someone disables the presets. What do you think?

Changed 2 months ago by GerdP

Attachment: 17567-rephrase.patch added

comment:20 Changed 2 months ago by GerdP

Sorry for the noise. The messages in TurnRestrictionTest should be preferred because some have a higher severity.
The patch 17567-rephrase.patch changes Unknown to Unexpected for both role and member type (relation in restriction) and removes the special case for location_hint. If I hear no complains I'll commit this tomorrow.

comment:21 Changed 2 months ago by GerdP

Resolution: fixed
Status: reopenedclosed

In 15003/josm:

fix #17567: apply 17567-rephrase.patch

comment:22 Changed 2 months ago by Klumbumbus

In 15004/josm:

  • see #17567 - make validator warnings more comprehensible
  • fix #17630 - warn about building:part=* together with building=*
  • fix #17593 - warn about sport=football

comment:23 Changed 2 months ago by Hb---

Resolution: fixed
Status: closedreopened

Please check the wording again.

"turn restriction" is a two word term.

The term "member type" in "Unexpected member type in turnrestriction" is questionable. According to "Type {0} of relation member with role {1} does not match accepted types {2} in preset {3}" which is used elsewhere in JOSM this string may be fine: "Unexpected type of relation member in turn restriction".

Last edited 2 months ago by Hb--- (previous) (diff)

comment:24 in reply to:  23 Changed 2 months ago by Klumbumbus

Replying to Hb---:

Please check the wording again.

"turn restriction" is a two word term.

I noticed this, but since turnrestrictions is already used several times in translatable strings I didn't want to throw away all the translations and instead went the way of consistency.

comment:25 Changed 2 months ago by Hb---

The translation source currently has 57 times the correct spelling and 9 times the bad spelling. Out of these 9 bad times:

  • the commit r14966 brought 1 new bad string ("Superfluous turnrestriction as \"from\" way is oneway")
  • the commit r15004 brought 3 new bad strings ("Unknown turnrestriction", "Unexpected role ''{0}'' in turnrestriction", "Unexpected member type in turnrestriction")

Not throwing away existing translations is a good reason to keep bad source strings. But it should be avoided to serve the translators another 4 bad strings introduced with the commits in this bug.

Please correct the new bad strings.

comment:26 Changed 2 months ago by GerdP

I agree. In r14966 I also kept the bad spelling without a good reason.

comment:27 Changed 2 months ago by Klumbumbus

Resolution: fixed
Status: reopenedclosed

was fixed in r15019
@Gerd: there was a typo the commit message (missing #) in r15019 therefore it didn't reference and close this ticket automatically. You can modify the commit message for such cases.

comment:28 Changed 2 months ago by GerdP

Ah, sorry, I know how to edit the commit message but I did not notice that typo. Do you want me to do it now?

comment:29 Changed 2 months ago by Klumbumbus

It's your choice.

comment:30 Changed 2 months ago by GerdP

In 15019/josm:

fix #17567: fix spelling turnrestriction -> turn restriction

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.