Modify

Opened 8 years ago

Last modified 4 months ago

#9679 reopened defect

[Incomplete Patch] Validator complains about key: *:lanes:conditional (etc.)

Reported by: alv Owned by: team
Priority: normal Milestone:
Component: Core validator Version: tested
Keywords: conditional lanes-tagging Cc: jc86035

Description (last modified by alv)

Since r6605 (see #9350) the validator has a too strict parser for any keys with the :conditional suffix. The conditional restrictions must be valid when combined with ":lanes" suffix, too?

e.g.:

goods:lanes:conditional = ||yes @ (Mo-Fr 09:00-15:00, 18:00-07:00; Sa-Su 24h)

(rightmost (bus) lane allowed for vans in the given timeframe. It could have :forward or :backward in the key, too. Also similar keys for other access modes. Example way: http://www.openstreetmap.org/way/29689100)

We also use:
traffic_sign:conditional = FI:361[100] @ (winter)
yet the validator complains about it. (They physically change the maxspeed sign for the winter.)

Another user ij_ wrote the attached patch, which fixes the test when ":lanes" is present in the key, and at least it compiled ok.

Attachments (2)

0001-Don-t-open-code-test-tree-for-components.patch (1.7 KB) - added by alv 8 years ago.
0002-Add-lanes-to-conditional-parsing.patch (1.7 KB) - added by alv 8 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 8 years ago by alv

Description: modified (diff)

comment:2 Changed 8 years ago by Don-vip

Milestone: 14.02
Summary: Validator complains about key: *:lanes:conditional (etc.)[Patch] Validator complains about key: *:lanes:conditional (etc.)

comment:3 Changed 8 years ago by simon04

The lanes "subkey" is nowhere mentioned at http://wiki.openstreetmap.org/wiki/Conditional_restrictions.

Similar to traffic_sign:conditional: According to the specification, the first part (traffic_sign) must either be a <restriction-type> or a <transportation mode>, but it isn't any of those two.

I woudn't change JOSM's validator unless the specification does reflect the usage of your two examples.

comment:4 Changed 8 years ago by Don-vip

Milestone: 14.02

comment:5 Changed 8 years ago by alv

Even if it's not specified in that general syntax formula (which is stricter than the definition above it), it's a sensible combination of the accepted way to tag lane specific restrictions http://wiki.openstreetmap.org/wiki/Lanes (That even says "This suffix is applicable to any existing <key>=<value> tag pair" - that was even documented before conditional restrictions proposal). And certainly not a wrong tag to use ("wrong syntax in key"). It might be necessary to decide which order is preferred, :lanes or :conditional as the last suffix. Now that I had some time to look at the code, it seems it doesn't check them at all if the :lanes was last in the key. Then, however, the "not in presets" test matches.

I do see the point of this check (e.g. maxspeed:hgv:forward:conditional vs. hgv:forward:maxspeed:conditional), but as it is now, it's inhibiting sensible "extension by usage". Somehow the validator went straight from "values not in presets" to strict parsing of anything with :conditional part in the key. I'd say the better way to check these current conditional tags would include checking if the parts of the key are a sensible subset of those as listed in the "general syntax", and only then if the order and the value are valid or not. But I realize it would require possibly quite a lot more thought to what makes the keys a part of the general syntax "family", and effort to the current straightforward test.

comment:6 Changed 7 years ago by ij

For the record: I realized later that the patches I made weren't complete as the :lanes values with | would need to be parsed too properly and the patches only solved the key part.

comment:7 Changed 6 years ago by simon04

Summary: [Patch] Validator complains about key: *:lanes:conditional (etc.)[Incomplete Patch] Validator complains about key: *:lanes:conditional (etc.)

comment:8 Changed 6 years ago by simon04

Resolution: wontfix
Status: newclosed

See comment:3

comment:9 Changed 5 years ago by simon04

Ticket #13742 has been marked as a duplicate of this ticket.

comment:10 Changed 5 years ago by simon04

Cc: jc86035 added

comment:11 Changed 5 years ago by jc86035

Why is this a wontfix? Lanes are mentioned directly in the Conditional restrictions wiki page § Evaluation of conflicting restrictions. (lanes:psv:conditional=* is used 132 times and lanes:bus:conditional 53 times. While that's not really a lot, we could always add more of them :P)

comment:12 Changed 5 years ago by simon04

Resolution: wontfix
Status: closedreopened

Still, the Wiki page lacks a description, see my comment:3.

comment:13 Changed 20 months ago by skyper

Keywords: lanes-tagging added; lanes removed

comment:14 Changed 4 months ago by skyper

See also #13651, #20832, #21163

comment:15 Changed 4 months ago by DodalerAfenger

The patch in #21163 should solve that issue, too

Modify Ticket

Change Properties
Set your email in Preferences
Action
as reopened The owner will remain team.
as The resolution will be set.
to The owner will be changed from team to the specified user.
The owner will change to alv
as duplicate The resolution will be set to duplicate.The specified ticket will be cross-referenced with this ticket

Add Comment


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

 
Note: See TracTickets for help on using tickets.