[PATCH] Complain about area=yes on major roads (like highway=primary area=yes)

[mkoniecz]

What steps will reproduce the problem?

1. create way, tag it with highway=primary area=yes noname=yes
2. run validator

What is the expected result?

Validator complains, maybe mentions that area:highway should be used for that

What happens instead?

Nothing

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

​http://overpass-turbo.eu/s/N0p finds 25k cases

I spotted problem during generating a custom map and encountering highway=unclassified area=yes where highway=service area=yes was a correct tagging.

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2019-10-08 23:08:20 +0200 (Tue, 08 Oct 2019)
Build-Date:2019-10-08 21:10:57
Revision:15445
Relative:URL: ^/trunk

Identification: JOSM/1.5 (15445 en) Linux Ubuntu 16.04.6 LTS
Memory Usage: 405 MB / 869 MB (152 MB allocated, but free)
Java version: 1.8.0_201-b09, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Screen: :0.0 1366x768
Maximum Screen Size: 1366x768
libcommons-logging-java: libcommons-logging-java:all-1.2-1+build1
fonts-noto: fonts-noto:-
Dataset consistency test: No problems found

Plugins:
+ OpeningHoursEditor (34977)
+ PicLayer (35104)
+ apache-commons (35092)
+ buildings_tools (35171)
+ continuosDownload (82)
+ ejml (35122)
+ geotools (35169)
+ imagery_offset_db (34908)
+ jts (35122)
+ log4j (34908)
+ measurement (35051)
+ reverter (35084)
+ 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/deprecated.mapcss to tag checker
- W: java.nio.file.NoSuchFileException: ${HOME}/Documents/install_moje/OSM software/josm/data/validator/deprecated.mapcss
- W: Failed to add ${HOME}/Documents/install_moje/OSM software/josm/data/validator/unnecessary.mapcss to tag checker
- W: java.nio.file.NoSuchFileException: ${HOME}/Documents/install_moje/OSM software/josm/data/validator/unnecessary.mapcss
- W: Failed to add ${HOME}/Documents/install_moje/OSM software/josm/data/validator/combinations.mapcss to tag checker
- W: java.nio.file.NoSuchFileException: ${HOME}/Documents/install_moje/OSM software/josm/data/validator/combinations.mapcss

[Klumbumbus]

with modifiable search area --> ​http://overpass-turbo.eu/s/N0y

[Gabe Reichenberger]

major highways with area=yes

[Gabe Reichenberger]

Added way validation test with the following rules:

1) Way has area=yes tag.
2) Way has highway tag >= tertiary (including links).

If both are true user gets warning that "area=yes" should not be used on major highways (tertiary or above).

[Gabe Reichenberger]

KEEP This has a very small update from the previous 18217.patch switching contains to equals in string comparison.

[GerdP]

Some remarks:

• Do you know the hasTag() mehthod?
• Why don't you use Set.contains()?
• I suggest tr("Found area=yes on major highway.") for the message to avoid the confusing upper case Area.

[GerdP]

BTW: The linked overpass query asks also for unclassified and residential. Without those the current count is just 145 major highways with area=yes.

[GerdP]

Hmm,sorry, I see no reason why this test needs java code instead of mapcss in either highways.mapcss or combinations.mapcss

[skyper]

Mmh, why not use mapcss instead of java? This should work, does it not?

area:closed2[highway =~ /^(motorway|motorway_link|trunk|trunk_link|primary|primary_link|secondary|secondary_link|tertiary|tertiary_link|residential|unclassified)$/][area=yes][highway] {
  throwError: tr("{0} together with {1} is invalid", "{3.tag}", "{4.tag}");
  group: tr("suspicious tag combination");
  suggestAlternative: "area:highway=*";
  assertMatch:   "area highway=trunk area=yes";
  assertNoMatch: "area highway=service area=yes";
  assertNoMatch: "way highway=trunk";
}

[anonymous]

Replying to GerdP:

Some remarks:

• Do you know the hasTag() mehthod?
• Why don't you use Set.contains()?
• I suggest tr("Found area=yes on major highway.") for the message to avoid the confusing upper case Area.

1) The hasTag and hasKey method have similar functionality.
2) For each element of the major highway tag set, an individual features’ highway tag must be an exact match.
3) I can adjust the message.

[anonymous]

Replying to GerdP:

BTW: The linked overpass query asks also for unclassified and residential. Without those the current count is just 145 major highways with area=yes.

145 globally? Talked with some expert mappers and apparently there are some cases where highway=<anything under tertiary> can be used in conjunction with area=yes? If this isn’t true I can definitely update.

[anonymous]

Replying to skyper:

Mmh, why not use mapcss instead of java? This should work, does it not?

area:closed2[highway =~ /^(motorway|motorway_link|trunk|trunk_link|primary|primary_link|secondary|secondary_link|tertiary|tertiary_link|residential|unclassified)$/][area=yes][highway] {
  throwError: tr("{0} together with {1} is invalid", "{3.tag}", "{4.tag}");
  group: tr("suspicious tag combination");
  suggestAlternative: "area:highway=*";
  assertMatch:   "area highway=trunk area=yes";
  assertNoMatch: "area highway=service area=yes";
  assertNoMatch: "way highway=trunk";
}

I’m not familiar with mapcss but if that works then sure, add it! I’ll be focusing more on adding via Java.

[GerdP]

Replying to anonym:

1) The hasTag and hasKey method have similar functionality.

No, hasTag("area", "yes") is simpler and easier to read than way.hasKey("area") && way.get("area").equals("yes").
Besides that sonarlint will complain about your code, it wants "yes".equals(way.get("area")) which also allows to omit the hasKey() check.

2) For each element of the major highway tag set, an individual features’ highway tag must be an exact match.

I would use something like this

if (hasTag("area", "yes") && MAJOR_HIGHWAYS.contains(way.get("highway"))) ...

3) I can adjust the message.

I think there is no need to adjust something in the patch, we always prefer to use mapcss rules where possible.

[anonymous]

Replying to GerdP:

Replying to anonym:

1) The hasTag and hasKey method have similar functionality.

No, hasTag("area", "yes") is simpler and easier to read than way.hasKey("area") && way.get("area").equals("yes").
Besides that sonarlint will complain about your code, it wants "yes".equals(way.get("area")) which also allows to omit the hasKey() check.

2) For each element of the major highway tag set, an individual features’ highway tag must be an exact match.

I would use something like this

if (hasTag("area", "yes") && MAJOR_HIGHWAYS.contains(way.get("highway"))) ...

3) I can adjust the message.

I think there is no need to adjust something in the patch, we always prefer to use mapcss rules where possible.

Oh that hasKey function. Forgot about that. Definitely better.

Cool, if that mapcss addition functions as intended ignore my Java

[skyper]

Replying to anonymous:

145 globally? Talked with some expert mappers and apparently there are some cases where highway=<anything under tertiary> can be used in conjunction with area=yes? If this isn’t true I can definitely update.

Please, share their thoughts and do you have an valid example?

Personally, I like to get completely rid of highway=* plus area=yes but that will take time. One major problem is the lack of support of area:highway=* in many software, e.g. #20102.

Replying to anonymous:

I’m not familiar with mapcss but if that works then sure, add it! I’ll be focusing more on adding via Java.

It is untested, written in a hurry and probably needs little polish, but should be enough for a proof of concept.

[anonymous]

Replying to skyper:

Replying to anonymous:

145 globally? Talked with some expert mappers and apparently there are some cases where highway=<anything under tertiary> can be used in conjunction with area=yes? If this isn’t true I can definitely update.

Please, share their thoughts and do you have an valid example?

Personally, I like to get completely rid of highway=* plus area=yes but that will take time. One major problem is the lack of support of area:highway=* in many software, e.g. #20102.

Replying to anonymous:

I’m not familiar with mapcss but if that works then sure, add it! I’ll be focusing more on adding via Java.

It is untested, written in a hurry and probably needs little polish, but should be enough for a proof of concept.

According to ​https://wiki.openstreetmap.org/wiki/Key:area looks like highway=pedestrian is the only highway tag that has a clear way of being used so looks like we can adjust the validation for everything except highway=pedestrian

I can test shortly as I learn mapcss real quick. :)

[GerdP]

I think highway=residential + area=yes is / was used often to map objects for which we now have place=square.

[mkoniecz]

looks like highway=pedestrian is the only highway tag that has a clear way of being used

highway=service highway=track highway=footway highway=path can also be combined with area=yes in a valid tagging describing actual features

Big part of highway=service + area=yes is incorrect tagging for the renderer, but not everything

[Gabe]

Replying to GerdP:

I think highway=residential + area=yes is / was used often to map objects for which we now have place=square.

Can you please point me to the destination where I can apply the mapcss?

[anonymous]

Replying to Gabe:

Replying to GerdP:

I think highway=residential + area=yes is / was used often to map objects for which we now have place=square.

Can you please point me to the destination where I can apply the mapcss?

found it, nvm!

[skyper]

As already mentioned, this test could be added to trunk/resources/data/validator/combinations.mapcss​ or trunk/resources/data/validator/highway.mapcss​. With the later a local class on top of the file for the highway values could be added.

For testing you can always place the rules in a local file and load it via Tag checker rules tab in the validator preferences.

[Gabe]

mapcss regarding area=yes with major highways

[Gabe]

Patch above seems to be working as intended. Only suggestion is to have ONLY the way's highway tag in the error message instead of all the invalid tags? What do you think? Show all invalid tags with area=yes or just the scenario specific tag?

[skyper]

Your patch so far only checks all ways. It misses MPs and I think we only should check closed ways (area:closed2).
Regarding the message, I would say either the specific tag or none at all which should group all objects.

[Gabe]

I have tried area:closed2 and am not seeing any warnings for closed areas. I loaded part of the Washington state boundary and edited the relation tags adding highway=primary and area=yes. No warnings after that.

[skyper]

patch

[skyper]

Ah, sorry, I was wrong. We need to split it into two lines as MPs do not need area=yes. Find attached patch​.

*[highway =~ /^(motorway|motorway_link|trunk|trunk_link|primary|primary_link|secondary|secondary_link|tertiary|tertiary_link|residential|unclassified)$/] {
set common_road;
}
way:closed.common_road[area=yes],
relation.common_road[type=multipolygon] {
  throwError: tr("Area with {0} above {1} are invalid", "highway=*", "highway=service");
  group: tr("suspicious tag combination");
  suggestAlternative: "area:highway=*";
  assertMatch:   "way highway=trunk area=yes";
  assertMatch:   "relation highway=trunk type=multipolygon";
  assertNoMatch: "way highway=service area=yes";
  assertNoMatch: "way highway=trunk";
}

[Gabe Reichenberger <v-garei@…>]

Replying to skyper:

Ah, sorry, I was wrong. We need to split it into two lines as MPs do not need area=yes. Find attached patch​.

*[highway =~ /^(motorway|motorway_link|trunk|trunk_link|primary|primary_link|secondary|secondary_link|tertiary|tertiary_link|residential|unclassified)$/] {
set common_road;
}
way:closed.common_road[area=yes],
relation.common_road[type=multipolygon] {
  throwError: tr("Area with {0} above {1} are invalid", "highway=*", "highway=service");
  group: tr("suspicious tag combination");
  suggestAlternative: "area:highway=*";
  assertMatch:   "way highway=trunk area=yes";
  assertMatch:   "relation highway=trunk type=multipolygon";
  assertNoMatch: "way highway=service area=yes";
  assertNoMatch: "way highway=trunk";
}

I see I see, according to the documentation I would have thought area:closed2 would have worked. So the patch is tested? Thanks for looking into this.

[skyper]

patch version 2: shorter class and correct grammar in message

[skyper]

josm_18217_v2.patch​ uses correct grammar in message and a shorter regex in the class.

Replying to Gabe Reichenberger <v-garei@…>:

I see I see, according to the documentation I would have thought area:closed2 would have worked.

Some functions only work with styles and not with validator test. This might be a bug, though. New ticket?

So the patch is tested?

Yes, my patches are usually tested, at least with some virtual test objects. BTW, the search dialog can be used with mapcss, e.g. for testing. Additionally there is the option to use "normal" JOSM Search in mapcss.

Thanks for looking into this.

I was not sure if you would have liked to solve it by yourself, so please, apologize if I stole the task.

[Gabe Reichenberger <v-garei@…>]

Replying to skyper:

josm_18217_v2.patch​ uses correct grammar in message and a shorter regex in the class.

Replying to Gabe Reichenberger <v-garei@…>:

I see I see, according to the documentation I would have thought area:closed2 would have worked.

Some functions only work with styles and not with validator test. This might be a bug, though. New ticket?

So the patch is tested?

Yes, my patches are usually tested, at least with some virtual test objects. BTW, the search dialog can be used with mapcss, e.g. for testing. Additionally there is the option to use "normal" JOSM Search in mapcss.

Thanks for looking into this.

I was not sure if you would have liked to solve it by yourself, so please, apologize if I stole the task.

Not a problem, just glad a solution was found! I will be working on a few more validations so I will get more chances.

[skyper]

Ping

[skyper]

For performance reasons, it might be better to work without defining the class first and taking comment:17 on #20902 and below into account we are probably better off with a general warning about relations with area=*.

Let's see where #20902 takes us.

[skyper]

version 3 without additional class

[skyper]

Damn, I should read my own patches more carefully as I always left out area=* for relations.

Find patch version 3​ without the extra class.
Should be ready for commit.

[Don-vip]

In 18104/josm:

fix #18217 - Complain about area=yes on major roads (like highway=primary area=yes) (patch by skyper)