Modify

Opened 3 years ago

Closed 10 months ago

#17607 closed enhancement (fixed)

[PATCH] handling `construction:highway`

Reported by: mkoniecz Owned by: team
Priority: normal Milestone: 21.07
Component: Core validator Version:
Keywords: template_report construction Cc: Penegal, AlfonZ, mkoniecz, Klumbumbus

Description (last modified by mkoniecz)

What steps will reproduce the problem?

  1. Create way with construction:highway=tertiary
  2. Create way with highway=construction construction=tertiary
  3. Run validator

What is the expected result?

Either it is rendered like highway=construction construction=tertiary or validator offers to fix it to highway=tertiary (or validator offers to add highway=tertiary). Or highway=construction construction=tertiary is migrated to construction:highway=tertiary.

Or maybe migrating to highway=construction only where it has broad support (road types)?

What happens instead?

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

https://taginfo.openstreetmap.org/keys/construction%3Ahighway#overview

Note that iD developers plan (proposed?) to start considering highway=construction as deprecated and start using construction:highway in https://github.com/openstreetmap/iD/issues/6168


URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2019-04-03 00:33:43 +0200 (Wed, 03 Apr 2019)
Build-Date:2019-04-03 01:30:50
Revision:14954
Relative:URL: ^/trunk

Identification: JOSM/1.5 (14954 en) Linux Ubuntu 16.04.6 LTS
Memory Usage: 435 MB / 869 MB (277 MB allocated, but free)
Java version: 1.8.0_201-b09, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Screen: :0.0 1920x1080
Maximum Screen Size: 1920x1080
Dataset consistency test: No problems found

Plugins:
+ OpeningHoursEditor (34867)
+ buildings_tools (34904)
+ continuosDownload (82)
+ imagery_offset_db (34867)
+ measurement (34867)
+ reverter (34961)
+ todo (30306)

Validator rules:
+ ${HOME}/Documents/install_moje/OSM software/josm/data/validator/deprecated.mapcss
+ ${HOME}/Documents/install_moje/OSM software/josm/data/validator/unnecessary.mapcss
+ ${HOME}/Documents/install_moje/OSM software/josm/data/validator/combinations.mapcss

Last errors/warnings:
- W: No configuration settings found.  Using hardcoded default values for all pools.
- W: Failed to add ${HOME}/Documents/install_moje/OSM software/josm/data/validator/unnecessary.mapcss to tag checker
- W: java.lang.IllegalArgumentException: Expecting n/node/w/way/r/relation/area, but got '"node'

Attachments (1)

17607.patch (3.1 KB) - added by reichg 12 months ago.
This patch addresses construction tags.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 3 years ago by mkoniecz

Description: modified (diff)

comment:2 Changed 3 years ago by mkoniecz

Description: modified (diff)

comment:3 Changed 3 years ago by mkoniecz

Description: modified (diff)

comment:4 Changed 3 years ago by Don-vip

Keywords: construction added

comment:5 Changed 12 months ago by reichg

Could we start a discussion this? Seems like construction:highway=* is not used very often? Only ~3k usages

comment:6 Changed 12 months ago by skyper

Life cycle tags are no problem according to the wiki (Key:construction), building, highway, railway and landuse use *=construction and construction=*.

The advantage of the lifecycle tag is that you can add all tags in the same way like:

access=private
construction:highway=path
construction:foot=designated
construction:bicycle=designated
construction:vehicle=no
construction:lit=yes
construction:surface=paving_stones
foot=destination
highway=construction
surface=unpaved

Atm, only highway and railway have support of construction=* in defaultpresets. The other two need an own preset and the matching of railway=construction needs to be recheck.

Long story short, how about warnings about construction:[building|highway|railway|landuse]=* without construction=* AND *=construction plus a warning about *=construction without construction=*. An autofix could be added but I would not remove construction:*=*.

*["construction:building"][!building][!construction],
*["construction:highway"][!highway][!construction],
*["construction:railway"][!railway][!construction],
*["construction:landuse"][!landuse][!construction] {
  ThrowWarning: tr("{0} without {1} and {2}", "{0.key}", "{1.key}={2.key}", "{2.key}");
  Group: missing tag;
}
*["construction:building"][building=construction][!construction],
*["construction:highway"][highway=construction][!construction],
*["construction:railway"][railway=construction][!construction],
*["construction:landuse"][landuse=construction][!construction] {
  ThrowWarning: tr("{1} with {0} but without {2}", "{0.key}", "{1.key}", "{2.key}");
  Group: missing tag;
}

Last edited 12 months ago by skyper (previous) (diff)

comment:7 in reply to:  6 ; Changed 12 months ago by reichg

Replying to skyper:

Life cycle tags are no problem according to the wiki (Key:construction), building, highway, railway and landuse use *=construction and construction=*.

The advantage of the lifecycle tag is that you can all tags in the same way like:

access=private
construction:highway=path
construction:foot=designated
construction:bicycle=designated
construction:vehicle=no
construction:lit=yes
construction:surface=paving_stones
foot=destination
highway=construction
surface=unpaved

Atm, only highway and railway have support of construction=* in defaultpresets. The other two need an own preset and the matching of railway=construction needs to be recheck.

Long story short, how about warnings about construction:[building|highway|railway|landuse]=* without construction=* AND *=construction plus a warning about *=construction without construction=*. An autofix could be added but I would not remove construction:*=*.

*["construction:building"][!building][!construction],
*["construction:highway"][!highway][!construction],
*["construction:railway"][!railway][!construction],
*["construction:landuse"][!landuse][!construction] {
  ThrowWarning: tr("{0} without {1} and {2}", "{0.key}", "{1.key}={2.key}", "{2.key}");
  Group: missing tag;
}
*["construction:building"][building=construction][!construction],
*["construction:highway"][highway=construction][!construction],
*["construction:railway"][railway=construction][!construction],
*["construction:landuse"][landuse=construction][!construction] {
  ThrowWarning: tr("{1} with {0} but without {2}", "{0.key}", "{1.key}", "{2.key}");
  Group: missing tag;
}

Yeah I can get behind this addition.

so for:

*["construction:building"][!building][!construction],
*["construction:highway"][!highway][!construction],
*["construction:railway"][!railway][!construction],
*["construction:landuse"][!landuse][!construction] {
  ThrowWarning: tr("{0} without {1} and {2}", "{0.key}", "{1.key}={2.key}", "{2.key}");
  Group: missing tag;

does !building, !highway, !railway, !landuse not also need to be !building=construction, !highway=construction, !railway=construction, and !landuse=construction like the second part of conditions?

comment:8 Changed 12 months ago by reichg

Having an issue catching [construction:*]. I have tried ["construction:*"], [/^construction:*$/], and [construction:*]. Is there a trick for the lifecycle prefix?

comment:9 in reply to:  7 ; Changed 12 months ago by skyper

Replying to reichg:

Yeah I can get behind this addition.

so for:

*["construction:building"][!building][!construction],
*["construction:highway"][!highway][!construction],
*["construction:railway"][!railway][!construction],
*["construction:landuse"][!landuse][!construction] {
  ThrowWarning: tr("{0} without {1} and {2}", "{0.key}", "{1.key}={2.key}", "{2.key}");
  Group: missing tag;

does !building, !highway, !railway, !landuse not also need to be !building=construction, !highway=construction, !railway=construction, and !landuse=construction like the second part of conditions?

Good point. What to do with conflicting tags with a different value than construction in primary tag but a present construction=* and/or construction:*=*. The later is probably valid for any object not just for the four and even for any lifecycle prefix.

Replying to reichg:

Having an issue catching [construction:*]. I have tried ["construction:*"], [/^construction:*$/], and [construction:*]. Is there a trick for the lifecycle prefix?

The star * is only a multiplier (arithmetic operator). An expression in front of it is needed. Right now, you are looking for zero to infinity colon : behind the letters.
A dot . is the expression for everything, e.g. [/^construction:.*$/] should do the trick, see https://docs.oracle.com/javase/tutorial/essential/regex/pre_char_classes.html

Not sure about this case, but sometimes, the eval expressions, regexp_test(), regexp_match() and tag_regex(), are the better choices/options, especially regarding autofix.

comment:10 Changed 12 months ago by skyper

Found some more cases to warn about for construction=*, see #20960.

comment:11 in reply to:  9 Changed 12 months ago by reichg

The star * is only a multiplier (arithmetic operator). An expression in front of it is needed. Right now, you are looking for zero to infinity colon : behind the letters.
A dot . is the expression for everything, e.g. [/^construction:.*$/] should do the trick, see https://docs.oracle.com/javase/tutorial/essential/regex/pre_char_classes.html

Yeah I just meant I couldn't get any of the following mapcss rules to be picked up by the validator. I tried giving a way the construction:highway tag without a highway=* AND construction=* tag but nothing showed up in the validator. I tried the variations /^construction:highway$/, "construction:highway" but still couldn't get the validator to pick up the construction:highway tag.

*[construction:building][!building][!construction],
*[construction:highway][!highway][!construction],
*[construction:railway][!railway][!construction],
*[construction:landuse][!landuse][!construction]
Last edited 12 months ago by reichg (previous) (diff)

comment:12 Changed 12 months ago by skyper

Damn, sorry, I have some stupid typos in my example.

  1. Keywords start with small letter (throwWarning: and group:)
  2. I forgot quotation marks for the group name (group: "missing tag";) and the tr()

The selectors work and are not the problem as a quick use of the expression in the search dialog proofs.

*["construction:building"][!building][!construction],
*["construction:highway"][!highway][!construction],
*["construction:railway"][!railway][!construction],
*["construction:landuse"][!landuse][!construction] {
  throwWarning: tr("{0} without {1} and {2}", "{0.key}", "{1.key}={2.key}", "{2.key}");
  group: tr("missing tag");
Last edited 12 months ago by skyper (previous) (diff)

comment:13 Changed 12 months ago by skyper

I went on and below is the result. Together with #20960 this should cover most. Last task is to properly fit it into combinations.mapcss. Please, test:

Edit: Changed the last rule once more.

/* missing or conflicting construction, see #17607 */
/* {0.key} without {1.key} and {2.key} */
*[construction:building][!building][!construction],
*[construction:highway][!highway][!construction],
*[construction:railway][!railway][!construction],
*[construction:landuse][!landuse][!construction] {
  throwWarning: tr("{0} without {1} and {2}", "{0.key}", "{1.key}", "{2.key}");
  group: tr("missing tag");
  fixAdd: "{1.key}=construction";
  fixAdd: "construction={0.value}";
  assertMatch:   "way construction:building=house";
  assertNoMatch: "way construction:building=house building=house ";
  assertNoMatch: "way construction:building=house construction=house";
}
/* {0.key} and {1.key} without {2.key} */
*[building=construction]["construction:building"][!construction],
*[highway=construction]["construction:highway"][!construction],
*[railway=construction]["construction:railway"][!construction],
*[landuse=construction]["construction:landuse"][!construction] {
  throwWarning:  tr("{0} together with {1} but without {2}", "{0.key}", "{1.key}", "{2.key}");
  group: tr("missing tag");
  fixAdd: "construction={1.value}";
  assertMatch:   "way construction:building=house building=construction";
  assertNoMatch: "way construction:building=house building=house ";
  assertNoMatch: "way construction:building=house construction=house";
}
/* {0.key} and {1.key} without {2.key} */
*[building]["construction:building"][!construction][building!=construction],
*[highway]["construction:highway"][!construction][highway!=construction],
*[railway]["construction:railway"][!construction][railway!=construction],
*[landuse]["construction:landuse"][!construction][landuse!=construction] {
  throwWarning: tr("{0} together with {1} and conflicting values plus no {2}", "{0.key}", "{1.key}", "{2.key}");
  group: tr("suspicious tag combination");
  assertMatch:   "way construction:building=house building=office";
  assertNoMatch: "way construction:building=house building=construction";
  assertNoMatch: "way construction:building=house construction=house";
}

*["construction:building"][construction][construction:building != *construction],
*["construction:highway"][construction][construction:highway != *construction],
*["construction:railway"][construction][construction:railway != *construction],
*["construction:landuse"][construction][construction:landuse != *construction] {
  throwWarning: tr("{0} together with {1} and conflicting values", "{0.key}", "{1.key}");
  group: tr("suspicious tag combination");
  assertMatch:   "way construction:building=house construction=office";
  assertNoMatch: "way construction:building=house construction=house";
}
Last edited 12 months ago by skyper (previous) (diff)

comment:14 in reply to:  12 Changed 12 months ago by reichg

Replying to skyper:

Damn, sorry, I have some stupid typos in my example.

  1. Keywords start with small letter (throwWarning: and group:)
  2. I forgot quotation marks for the group name (group: "missing tag";) and the tr()

The selectors work and are not the problem as a quick use of the expression in the search dialog proofs.

*["construction:building"][!building][!construction],
*["construction:highway"][!highway][!construction],
*["construction:railway"][!railway][!construction],
*["construction:landuse"][!landuse][!construction] {
  throwWarning: tr("{0} without {1} and {2}", "{0.key}", "{1.key}={2.key}", "{2.key}");
  group: tr("missing tag");

Ah I do not know why I didn't catch that! Makes perfect sense why the validator didn't pick them up.

Changed 12 months ago by reichg

Attachment: 17607.patch added

This patch addresses construction tags.

comment:15 Changed 12 months ago by reichg

patch 17607 has been tested for each of the 4 rules. Seems to be working as we want.

comment:16 Changed 12 months ago by reichg

Summary: handling `construction:highway`[PATCH] handling `construction:highway`

comment:17 Changed 11 months ago by reichg

Can this patch be merged?

comment:18 Changed 11 months ago by reichg

Still need a maintainer!

comment:19 Changed 11 months ago by skyper

Cc: Klumbumbus added

comment:20 Changed 11 months ago by reichg

Cc: Penegal AlfonZ mkoniecz added

comment:21 Changed 11 months ago by skyper

@reichg
Just to give you some info.
Only Klumbumbus is an active core maintainer besides stoecker, Don-vip, simon04 and GerdP. Some more people have write access but will probably not commit patches except for certain parts. Even the first five usually have certain parts they are familiar with.

Additionally, the reporter and any person who leaves a comment are silently added to cc.

comment:22 Changed 11 months ago by skyper

Milestone: 21.06

comment:23 in reply to:  21 Changed 11 months ago by reichg

Replying to skyper:

@reichg
Just to give you some info.
Only Klumbumbus is an active core maintainer besides stoecker, Don-vip, simon04 and GerdP. Some more people have write access but will probably not commit patches except for certain parts. Even the first five usually have certain parts they are familiar with.

Additionally, the reporter and any person who leaves a comment are silently added to cc.

This is good to know! Thanks!

comment:24 Changed 11 months ago by reichg

Can we get this merged please?

comment:25 Changed 11 months ago by Don-vip

Milestone: 21.0621.07

comment:26 Changed 10 months ago by Don-vip

Resolution: fixed
Status: newclosed

In 18103/josm:

fix #17607 - construction:highway validation (patch by reichg)

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.