Modify

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#20987 closed enhancement (fixed)

Adding a validator rule: check key `turn` for wrong value `straight` instead of `through` [patch]

Reported by: anonymous Owned by: team
Priority: normal Milestone: 21.06
Component: Core validator Version:
Keywords: turn lanes values Cc:

Description

What steps will reproduce the problem?

  1. Adding tag with key:turn:* and value:straight

What is the expected result?

Send out a information/warning/error message: "Please don't use straight value for turn-scheme and change to through value

What happens instead?

No message

Attachments (0)

Change History (17)

comment:2 Changed 2 years ago by skyper

Keywords: turn lanes values added; validator rule removed

Mmh, you'll get a informal warning with the external preset LaneAttributes installed:

Presets do not contain property value - Value 'straight' for key 'turn' not in presets.

but there is a problem with this test and multiselect, see #19519.
On the other hand turn:lanes=straight|right does not produce any warning, even though the preset does not contain this value (combo).

So we need a check for proper turn values for all turn[:*]=* tags not just turn=* and ignore the keys for the test above.

comment:3 Changed 2 years ago by skyper

The biggest challenge is probably the need of two splits for multiple values within turn:lanes[:*]=* as turn:lanes:forward=sharp_left;left|left;through;slight_right|slight_right;right is a valid tag.

  • Keys to check are [/^turn(:both_ways)?(:forward|:backward)?$/] and [/^turn:lanes(:both_ways)?(:forward|:backward)?$/]
  • Values are /^((sharp_|slight_)?(left|right)|reverse|through)$/

comment:4 Changed 2 years ago by Klumbumbus

As the number of values with straight instead of through in the database is very low I don't think we need a specialized warning à la "...use through instead of straight". But we can add a general warning. What about the following? It doesn't catch all possible errors, but should find some while hopefully being pretty false positive safe. And we also already have https://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/data/validation/tests/Lanes.java

way[turn                ][turn                 !~ /^((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through|none|;)+$/],
way[turn:forward        ][turn:forward         !~ /^((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through|none|;)+$/],
way[turn:backward       ][turn:backward        !~ /^((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through|none|;)+$/],
way[turn:both_ways      ][turn:both_ways       !~ /^((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through|none|;)+$/],
way[turn:lanes          ][turn:lanes           !~ /^((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through|none|;|\|)+$/],
way[turn:lanes:forward  ][turn:lanes:forward   !~ /^((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through|none|;|\|)+$/],
way[turn:lanes:backward ][turn:lanes:backward  !~ /^((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through|none|;|\|)+$/],
way[turn:lanes:both_ways][turn:lanes:both_ways !~ /^((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through|none|;|\|)+$/] {
  throwWarning: tr("unusual value of {0}", "{0.key}");
  assertMatch: "way turn=straight"; /* through would be correct */
  assertMatch: "way turn=slight_reverse"; /* wrong value */
  assertMatch: "way turn=through|right"; /*  :lanes missing in key */
  assertNoMatch: "way turn=through;right";
  assertMatch: "way turn:lanes:forward=straight|right"; /* through would be correct */
  assertMatch: "way turn:lanes:forward=slight_reverse|right"; /* wrong value */
  assertNoMatch: "way turn:lanes:forward=sharp_left;left|left;through;slight_right|slight_right;right";
}

comment:5 Changed 2 years ago by Klumbumbus

Milestone: 21.06
Summary: Adding a validator rule: check key `turn` for wrong value `straight` instead of `through`Adding a validator rule: check key `turn` for wrong value `straight` instead of `through` [patch]

comment:6 in reply to:  4 Changed 2 years ago by skyper

Replying to Klumbumbus:
+1,
Nice, you catch my missing values.

comment:7 Changed 2 years ago by Klumbumbus

Resolution: fixed
Status: newclosed

In 17935/josm:

fix #20987 - Warn about unusual values of turn(:lanes(:forward|:backward|:both_ways)) (based on regex by skyper)

comment:8 in reply to:  7 Changed 2 years ago by skyper

Resolution: fixed
Status: closedreopened

Replying to Klumbumbus:

In 17935/josm:

fix #20987 - Warn about unusual values of turn(:lanes(:forward|:backward|:both_ways)) (based on regex by skyper)

Thanks for the credit.

Arrg, sorry, I was too fast. none is only valid as solo value and there is no need for empty values. Additionally, turn(:lanes)?:both_ways:(forward|backward) is very seldom but a possible tag. Below might be better

way[turn                         ][turn                          !~ /^(none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*)$/],
way[turn:forward                 ][turn:forward                  !~ /^(none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*)$/],
way[turn:backward                ][turn:backward                 !~ /^(none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*)$/],
way[turn:both_ways               ][turn:both_ways                !~ /^(none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*)$/],
way[turn:both_ways:forward       ][turn:both_ways:forward        !~ /^(none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*)$/],
way[turn:both_ways:backward      ][turn:both_ways:backward       !~ /^(none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*)$/],
way[turn:lanes                   ][turn:lanes                    !~ /^(none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*)(\|(none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*))*$/],
way[turn:lanes:forward           ][turn:lanes:forward            !~ /^(none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*)(\|(none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*))*$/],
way[turn:lanes:backward          ][turn:lanes:backward           !~ /^(none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*)(\|(none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*))*$/],
way[turn:lanes:both_ways         ][turn:lanes:both_ways          !~ /^(none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*)(\|(none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*))*$/], 
way[turn:lanes:both_ways:forward ][turn:lanes:both_ways:forward  !~ /^(none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*)(\|(none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*))*$/],
way[turn:lanes:both_ways:backward][turn:lanes:both_ways:backward !~ /^(none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*)(\|(none|((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through)(;((sharp_|slight_|merge_to_|slide_)?(left|right)|reverse|through))*))*$/] {
  throwWarning: tr("unusual value of {0}", "{0.key}");
  assertMatch: "way turn=straight"; /* through would be correct */
  assertMatch: "way turn=slight_reverse"; /* wrong value */
  assertMatch: "way turn=through|right"; /*  :lanes missing in key */
  assertNoMatch: "way turn=through;right";
  assertMatch: "way turn:lanes:forward=straight|right"; /* through would be correct */
  assertMatch: "way turn:lanes:forward=slight_reverse|right"; /* wrong value */
  assertNoMatch: "way turn:lanes:forward=sharp_left;left|left;through;slight_right|slight_right;right";
  assertMatch: "way turn:lanes=left;none|right"; /* "none" needs to be solo value */
}
Last edited 2 years ago by skyper (previous) (diff)

comment:9 Changed 2 years ago by Klumbumbus

In 17937/josm:

see #20987 - Better regex for warnings about unusual values of turn(:lanes(:forward|:backward|:both_ways)) (patch by skyper)

comment:10 Changed 2 years ago by Klumbumbus

Thx.

comment:11 Changed 2 years ago by Klumbumbus

Resolution: fixed
Status: reopenedclosed

comment:12 in reply to:  10 ; Changed 2 years ago by skyper

Replying to Klumbumbus:

Thx.

No big deal. :lanes-tagging is one of my interest since many year, sadly, imagic lost interest and I am not that familiar with style syntax, yet. (Styles/Lane_and_Road_Attributes would benefit from some updates/enhancements.)

Thank you for committing.

comment:13 in reply to:  12 ; Changed 2 years ago by Klumbumbus

Replying to skyper:

(Styles/Lane_and_Road_Attributes would benefit from some updates/enhancements.)

That style is so complex, I don't want to dig into it :)

comment:14 in reply to:  13 Changed 2 years ago by skyper

Replying to Klumbumbus:

Replying to skyper:

(Styles/Lane_and_Road_Attributes would benefit from some updates/enhancements.)

That style is so complex, I don't want to dig into it :)

Yes, that is my problem, too. Not the easiest challenge to start learning style syntax.

comment:15 Changed 2 years ago by Famlam

Regression (reported in #21106)
The new regex doesn't allow for none to be omitted, i.e. |right equals none|right

comment:16 Changed 2 years ago by Klumbumbus

In 18022/josm:

fix #21106, see #20987 - Don't warn about empty values in turn:lanes tags (patch by skyper)

comment:17 Changed 2 years ago by anonymous

Thread opener says: Thank you all for fixing :-)

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.