Opened 4 years ago
Closed 4 years ago
#21106 closed defect (fixed)
[patch] False positive: unusual value of turn:lanes with turn:lanes=|right (or anything with no lanes and no `none`)
Reported by: | Famlam | Owned by: | team |
---|---|---|---|
Priority: | normal | Milestone: | 21.07 |
Component: | Core validator | Version: | tested |
Keywords: | template_report turn:lanes none | Cc: |
Description
What steps will reproduce the problem?
- Validate way 963761819
What is the expected result?
No warning, it has one lane without marking and one that is marked as "right"
What happens instead?
"Unusual value of turn:lanes" (turn:lanes=|right
)
Please provide any additional information below. Attach a screenshot if possible.
Regression of #20987. In turn:lanes none
can be omitted
URL:https://josm.openstreetmap.de/svn/trunk Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b Last:Changed Date: 2021-07-12 02:41:41 +0200 (Mon, 12 Jul 2021) Build-Date:2021-07-12 00:42:49 Revision:18004 Relative:URL: ^/trunk Identification: JOSM/1.5 (18004 nl) Windows 10 64-Bit OS Build number: Windows 10 Home 2009 (19043) Memory Usage: 1679 MB / 1820 MB (407 MB allocated, but free) Java version: 1.8.0_291-b10, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM Look and Feel: com.sun.java.swing.plaf.windows.WindowsLookAndFeel Screen: \Display0 1920×1080 (scaling 1.00×1.00) Maximum Screen Size: 1920×1080 Best cursor sizes: 16×16→32×32, 32×32→32×32 System property file.encoding: Cp1252 System property sun.jnu.encoding: Cp1252 Locale info: nl_NL Numbers with default locale: 1234567890 -> 1234567890 Dataset consistency test: No problems found Plugins: + OpeningHoursEditor (35640) + SimplifyArea (35640) + imagery_offset_db (35640) + pt_assistant (1ff2e15) + reverter (35732) + tageditor (35640) + turnlanes-tagging (288) + undelete (35640) + utilsplugin2 (35691) Tagging presets: + http://mijndev.openstreetmap.nl/~allroads/JOSM/Presets/NL-Fiets.zip Map paint styles: - https://josm.openstreetmap.de/josmfile?page=Styles/Lane_and_Road_Attributes&zip=1 - https://josm.openstreetmap.de/josmfile?page=Styles/PublicTransport&zip=1 + %UserProfile%\Documents\tijdelijke bestanden\josm-eigen.mappaint.mapcss + https://josm.openstreetmap.de/josmfile?page=Styles/Sidewalks&zip=1 Validator rules: + %UserProfile%\Documents\tijdelijke bestanden\josm-eigen.validator.mapcss Last errors/warnings: - 00013.187 E: Lokaliseren van afbeelding 'bus.png' mislukt - 07048.483 W: JCS - Silent failure during download: https://service.pdok.nl/hwh/luchtfotorgb/wms/v1_0?FORMAT=image/jpeg&VERSION=1.3.0&SERVICE=WMS&REQUEST=GetMap&LAYERS=2021_quick_orthoHR&STYLES=&CRS=EPSG:3857&WIDTH=512&HEIGHT=512&BBOX=654224.5250928,6791735.7112510,654300.9621211,6791812.1482793 - 07089.806 W: JCS - Silent failure during download: https://service.pdok.nl/hwh/luchtfotorgb/wms/v1_0?FORMAT=image/jpeg&VERSION=1.3.0&SERVICE=WMS&REQUEST=GetMap&LAYERS=2021_quick_orthoHR&STYLES=&CRS=EPSG:3857&WIDTH=512&HEIGHT=512&BBOX=654300.9621211,6791582.8371945,654377.3991494,6791659.2742228 - 07091.297 W: JCS - Silent failure during download: https://service.pdok.nl/hwh/luchtfotorgb/wms/v1_0?FORMAT=image/jpeg&VERSION=1.3.0&SERVICE=WMS&REQUEST=GetMap&LAYERS=2021_quick_orthoHR&STYLES=&CRS=EPSG:3857&WIDTH=512&HEIGHT=512&BBOX=654300.9621211,6791659.2742228,654377.3991494,6791735.7112510 - 07091.913 W: JCS - Silent failure during download: https://service.pdok.nl/hwh/luchtfotorgb/wms/v1_0?FORMAT=image/jpeg&VERSION=1.3.0&SERVICE=WMS&REQUEST=GetMap&LAYERS=2021_quick_orthoHR&STYLES=&CRS=EPSG:3857&WIDTH=512&HEIGHT=512&BBOX=654300.9621211,6791506.4001662,654377.3991494,6791582.8371945 - 08044.652 W: Verwijderde of verplaatste objecten - <html>Er staan 3 objecten in uw lokale gegevensset die zouden kunnen worden verwijderd op de server.<br>Indien u dit later probeert te verwijderen of bij te werken zal de server zeer waarschijnlijk een melding van een conflict geven.<br>Klik op <strong>Controleer op de server</strong> om de status van deze objecten op de server te controleren.<br>Klik op <strong>Negeren</strong> om te negeren.</html>
Attachments (1)
Change History (12)
follow-up: 2 comment:1 by , 4 years ago
Keywords: | regression removed |
---|
comment:2 by , 4 years ago
follow-up: 5 comment:4 by , 4 years ago
Hmm, it's a very widely used, documented scheme to omit none. (Personally I tend to remove none
s when I encounter them, they're quite rare compared to ""). So although JOSM can encourage none over "", I don't believe it should flag it as bad.
See https://taginfo.openstreetmap.org/keys/turn:lanes#values
P.s. it wasn't present in the previous rules, so hence the regression. But that's just a word, bug/false positive is enough ;)
follow-up: 7 comment:5 by , 4 years ago
Replying to Famlam:
Hmm, it's a very widely used, documented scheme to omit none.
All examples on the wiki include none
.
(Personally I tend to remove
none
s when I encounter them, they're quite rare compared to ).
Please, don't do that.
So although JOSM can encourage none over , I don't believe it should flag it as bad.
Do you mean it should only be an informal warning for empty value parts in turn:lanes[:*]
? Then I would rather remove it without any warning for empty values.
See https://taginfo.openstreetmap.org/keys/turn:lanes#values
Yes, it was in my external preset (Presets/LaneAttributes) for a long time until I changed it some weeks ago. Personally, I used to omit none
, as-well.
On the other hand, I know that some users did misunderstand it and most of the issues of missing values (other rule) I find in the wild are caused by users removing the last |
without any value after it.
P.s. it wasn't present in the previous rules, so hence the regression. But that's just a word, bug/false positive is enough ;)
There was no rule checking on turn
values prior to r17935.
All together, the fix/change would be rather easy, though, I'd like to hear some more opinions.
follow-up: 8 comment:6 by , 4 years ago
Then I would rather remove it without any warning for empty values.
I would definitely be in favor of removing the warning altogether (and at minimum for splitting it up, as then I can ignore this validator rule while still having the validation part for any real error), but I could live with an info level "warning" about "Prefer 'none' over <nothing> for readability purposes".
On the other hand, I know that some users did misunderstand it and most of the issues of missing values (other rule) I find in the wild are caused by users removing the last | without any value after it.
Which is what validation of lanes=*
vs. the number of |
s could help with. If desired, it's quite a large chunk (more suitable for Java I guess as then you can just loop over it):
way[lanes=2][/.+:lanes$/=~/^[^\|]*$/], way[lanes=3][/.+:lanes$/=~/^([^\|]*\|){0,1}[^\|]*$/], way[lanes=4][/.+:lanes$/=~/^([^\|]*\|){0,2}[^\|]*$/], way[lanes=5][/.+:lanes$/=~/^([^\|]*\|){0,3}[^\|]*$/], way[lanes=6][/.+:lanes$/=~/^([^\|]*\|){0,4}[^\|]*$/], way[lanes=7][/.+:lanes$/=~/^([^\|]*\|){0,5}[^\|]*$/], way[lanes=8][/.+:lanes$/=~/^([^\|]*\|){0,6}[^\|]*$/], way[lanes=9][/.+:lanes$/=~/^([^\|]*\|){0,7}[^\|]*$/], way[lanes:forward=2][/.+:lanes:forward$/=~/^[^\|]*$/], way[lanes:forward=3][/.+:lanes:forward$/=~/^([^\|]*\|){0,1}[^\|]*$/], way[lanes:forward=4][/.+:lanes:forward$/=~/^([^\|]*\|){0,2}[^\|]*$/], way[lanes:forward=5][/.+:lanes:forward$/=~/^([^\|]*\|){0,3}[^\|]*$/], way[lanes:forward=6][/.+:lanes:forward$/=~/^([^\|]*\|){0,4}[^\|]*$/], way[lanes:forward=7][/.+:lanes:forward$/=~/^([^\|]*\|){0,5}[^\|]*$/], way[lanes:forward=8][/.+:lanes:forward$/=~/^([^\|]*\|){0,6}[^\|]*$/], way[lanes:forward=9][/.+:lanes:forward$/=~/^([^\|]*\|){0,7}[^\|]*$/], way[lanes:backward=2][/.+:lanes:backward$/=~/^[^\|]*$/], way[lanes:backward=3][/.+:lanes:backward$/=~/^([^\|]*\|){0,1}[^\|]*$/], way[lanes:backward=4][/.+:lanes:backward$/=~/^([^\|]*\|){0,2}[^\|]*$/], way[lanes:backward=5][/.+:lanes:backward$/=~/^([^\|]*\|){0,3}[^\|]*$/], way[lanes:backward=6][/.+:lanes:backward$/=~/^([^\|]*\|){0,4}[^\|]*$/], way[lanes:backward=7][/.+:lanes:backward$/=~/^([^\|]*\|){0,5}[^\|]*$/], way[lanes:backward=8][/.+:lanes:backward$/=~/^([^\|]*\|){0,6}[^\|]*$/], way[lanes:backward=9][/.+:lanes:backward$/=~/^([^\|]*\|){0,7}[^\|]*$/], way[lanes:both_ways=2][/.+:lanes:both_ways$/=~/^[^\|]*$/], way[lanes:both_ways=3][/.+:lanes:both_ways$/=~/^([^\|]*\|){0,1}[^\|]*$/], way[lanes:both_ways=4][/.+:lanes:both_ways$/=~/^([^\|]*\|){0,2}[^\|]*$/], way[lanes:both_ways=5][/.+:lanes:both_ways$/=~/^([^\|]*\|){0,3}[^\|]*$/], way[lanes:both_ways=6][/.+:lanes:both_ways$/=~/^([^\|]*\|){0,4}[^\|]*$/], way[lanes:both_ways=7][/.+:lanes:both_ways$/=~/^([^\|]*\|){0,5}[^\|]*$/], way[lanes:both_ways=8][/.+:lanes:both_ways$/=~/^([^\|]*\|){0,6}[^\|]*$/], way[lanes:both_ways=9][/.+:lanes:both_ways$/=~/^([^\|]*\|){0,7}[^\|]*$/] { throwWarning: tr("{0} is larger than the number of lanes in a *:lanes tag", "{0.tag}"); group: tr("eigen filters"); assertMatch: "way highway=primary oneway=yes lanes=2 bus:lanes=no"; assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=no"; assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=|no"; assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=no|"; assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=|no|"; assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=|no| turn:lanes=left|through|through|right"; assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=no||"; assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=||no"; assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=yes|no"; assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=no|yes"; assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=yes|no|yes"; assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=no|yes|yes"; assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=yes|yes|no"; assertNoMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=no|yes|yes|yes"; assertNoMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=no|||"; assertNoMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=|no||"; assertNoMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=|||no"; assertNoMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=|||no turn:lanes=left|through|through|right"; assertNoMatch: "way highway=primary oneway=yes lanes=4"; } way[lanes=1][/.+:lanes$/=~/\|/], way[lanes=2][/.+:lanes$/=~/^(.*\|){2}/], way[lanes=3][/.+:lanes$/=~/^(.*\|){3}/], way[lanes=4][/.+:lanes$/=~/^(.*\|){4}/], way[lanes=5][/.+:lanes$/=~/^(.*\|){5}/], way[lanes=6][/.+:lanes$/=~/^(.*\|){6}/], way[lanes=7][/.+:lanes$/=~/^(.*\|){7}/], way[lanes=8][/.+:lanes$/=~/^(.*\|){8}/], way[lanes=9][/.+:lanes$/=~/^(.*\|){9}/], way[lanes:forward=1][/.+:lanes:forward$/=~/\|/], way[lanes:forward=2][/.+:lanes:forward$/=~/^(.*\|){2}/], way[lanes:forward=3][/.+:lanes:forward$/=~/^(.*\|){3}/], way[lanes:forward=4][/.+:lanes:forward$/=~/^(.*\|){4}/], way[lanes:forward=5][/.+:lanes:forward$/=~/^(.*\|){5}/], way[lanes:forward=6][/.+:lanes:forward$/=~/^(.*\|){6}/], way[lanes:forward=7][/.+:lanes:forward$/=~/^(.*\|){7}/], way[lanes:forward=8][/.+:lanes:forward$/=~/^(.*\|){8}/], way[lanes:forward=9][/.+:lanes:forward$/=~/^(.*\|){9}/], way[lanes:backward=1][/.+:lanes:backward$/=~/\|/], way[lanes:backward=2][/.+:lanes:backward$/=~/^(.*\|){2}/], way[lanes:backward=3][/.+:lanes:backward$/=~/^(.*\|){3}/], way[lanes:backward=4][/.+:lanes:backward$/=~/^(.*\|){4}/], way[lanes:backward=5][/.+:lanes:backward$/=~/^(.*\|){5}/], way[lanes:backward=6][/.+:lanes:backward$/=~/^(.*\|){6}/], way[lanes:backward=7][/.+:lanes:backward$/=~/^(.*\|){7}/], way[lanes:backward=8][/.+:lanes:backward$/=~/^(.*\|){8}/], way[lanes:backward=9][/.+:lanes:backward$/=~/^(.*\|){9}/], way[lanes:both_ways=1][/.+:lanes:both_ways$/=~/\|/], way[lanes:both_ways=2][/.+:lanes:both_ways$/=~/^(.*\|){2}/], way[lanes:both_ways=3][/.+:lanes:both_ways$/=~/^(.*\|){3}/], way[lanes:both_ways=4][/.+:lanes:both_ways$/=~/^(.*\|){4}/], way[lanes:both_ways=5][/.+:lanes:both_ways$/=~/^(.*\|){5}/], way[lanes:both_ways=6][/.+:lanes:both_ways$/=~/^(.*\|){6}/], way[lanes:both_ways=7][/.+:lanes:both_ways$/=~/^(.*\|){7}/], way[lanes:both_ways=8][/.+:lanes:both_ways$/=~/^(.*\|){8}/], way[lanes:both_ways=9][/.+:lanes:both_ways$/=~/^(.*\|){9}/] { throwWarning: tr("{0} is smaller than the number of lanes in a *:lanes tag", "{0.tag}"); group: tr("eigen filters"); assertMatch: "way highway=primary oneway=yes lanes=1 bus:lanes=|no"; assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=||||no"; assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=|||no|"; assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=||no||"; assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=|no|||"; assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=no||||"; assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=yes|yes|yes|yes|no"; assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=yes|yes|yes|no|yes"; assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=yes|yes|no|yes|yes"; assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=yes|no|yes|yes|yes"; assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=no|yes|yes|yes|yes"; assertMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=no|yes|yes|yes|yes turn:lanes=left|through|through|right"; assertNoMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=no|yes|yes|yes"; assertNoMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=no|||"; assertNoMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=|no||"; assertNoMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=|||no"; assertNoMatch: "way highway=primary oneway=yes lanes=4 bus:lanes=|||no turn:lanes=left|through|through|right"; assertNoMatch: "way highway=primary oneway=yes lanes=4"; }
But that's off-topic. Main point in my reply to this part is that a widely used syntax shouldn't be "forbidden", even if some users don't understand it (like many things: you have to learn it first). I guess TagInfo gives enough clues about the opinions of others?
comment:7 by , 4 years ago
Milestone: | → 21.07 |
---|
Replying to skyper:
I'd like to hear some more opinions.
There should be no warning for omitted "none" also not on info level.
comment:8 by , 4 years ago
Replying to Klumbumbus:
Replying to skyper:
I'd like to hear some more opinions.
There should be no warning for omitted "none" also not on info level.
Ok, empty values need to be added to all turn:lanes*
. Below should do the trick (untested)
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))*))*$/] {
Replying to Famlam:
On the other hand, I know that some users did misunderstand it and most of the issues of missing values (other rule) I find in the wild are caused by users removing the last | without any value after it.
Which is what validation of
lanes=*
vs. the number of|
s could help with. If desired, it's quite a large chunk (more suitable for Java I guess as then you can just loop over it):
There is already a test for this problem, Number of lanes greater than *:lanes
but it might need some tweaks, see #17172, #18488 and #19649 plus #19095, #19653, and #20236.
Be careful, as only lanes for cars are counted in lanes[:backward/forward/both_ways]
but all lanes for *:lanes[:*]
.
comment:9 by , 4 years ago
Summary: | False positive: unusual value of turn:lanes with turn:lanes=|right (or anything with no lanes and no `none`) → [patch] False positive: unusual value of turn:lanes with turn:lanes=|right (or anything with no lanes and no `none`) |
---|
Replying to skyper:
Ok, empty values need to be added to all
turn:lanes*
. Below should do the trick (untested)
I can confirm that patch solves this issue! Thanks Skyper!
Perhaps also add, to prevent future issues:
assertNoMatch: "way turn:lanes=||through|";
Be careful, as only lanes for cars are counted in
lanes[:backward/forward/both_ways]
but all lanes for*:lanes[:*]
.
Thanks!
comment:10 by , 4 years ago
I attached a proper patch file which adds the empty values for turn:lanes*
and adjusted the assertNoMatch
to include empty values.
You are right, but I think it is better to add
none
. Have to update the preset, though, and maybe the message should be changed to reflect the empty value e.g. missingnone
.Anyway, it is no regression as the rule was just added.