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

[anonymous]

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

[anonymous]

+ wiki ref:
​https://wiki.openstreetmap.org/wiki/Key:turn

[skyper]

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.

[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)$/

[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";
}

[skyper]

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

[Klumbumbus]

In 17935/josm:

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

[skyper]

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 */
}

[Klumbumbus]

In 17937/josm:

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

[Klumbumbus]

Thx.

[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.

[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 :)

[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.

[Famlam]

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

[Klumbumbus]

In 18022/josm:

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

[anonymous]

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