Modify

Opened 16 months ago

Closed 4 months ago

#21235 closed enhancement (fixed)

[PATCH] Stop whitelisting footway=*, and add footway=separate to blacklist candidates for sidewalk=*

Reported by: JeroenHoek Owned by: team
Priority: normal Milestone: 22.07
Component: Core validator Version:
Keywords: footway sidewalk Cc:

Description (last modified by JeroenHoek)

To resolve #10851, #10976, #15439, and #19389 the validator checks if ways have footway=* set with one of the values typically used for sidewalk=*. This seems to be a fairly common mistake, so it's good to have a warning here.

Issues

There are two minor issues with the current validation approach I would like to resolve.

footway=separate is missing

The validator checks for left, right, both, none, and no. separate is missing from this list.

Whitelist blocks all other possible footway values except for three

While the above values are clear examples of misstagging, footway is not an invalid key, and is used to provide a refining of highway=footway. The validator acknowledges this for access_aisle, crossing, and sidewalk, but excludes everything else. This seems too strict compared to the other rules — we don't whitelist values for highway=* for example. Mappers can (and do) create new values, and some are gaining broader acceptance. This check seems superfluous with the checks for values that probably mean sidewalk=* in place.

Patch

In the attached patch:

  • A check for footway=separate is added.
  • The whitelist for footway=* is removed.

Are there any issues I am overlooking or reintroducing with this patch?

Attachments (1)

0001-Fix-minor-issues-with-footway-validation.patch (3.1 KB) - added by JeroenHoek 5 months ago.
Patch

Download all attachments as: .zip

Change History (24)

comment:1 Changed 16 months ago by JeroenHoek

Description: modified (diff)

comment:2 Changed 16 months ago by JeroenHoek

Component: CoreCore validator
Type: defectenhancement

comment:3 Changed 16 months ago by JeroenHoek

Description: modified (diff)

comment:4 Changed 15 months ago by skyper

According to #21242, sidewalk=none should be replaced by sidewalk=no, so JOSM is correct with the special treatment.

comment:5 Changed 15 months ago by skyper

Keywords: footway sidewalk added

comment:6 Changed 15 months ago by anonymous

Really? This isn't reflected in the current documentation of sidewalk=*. I'm not seeing any recent mention on the Taggin-ML either.

I'm not opposed to deprecating sidewalk=none, but was this discussed somewhere else than on the Taggin-ML and the wiki-page for sidewalk=*?

comment:7 Changed 15 months ago by JeroenHoek

(Comment by me.)

comment:9 Changed 15 months ago by mkoniecz

that was me, sorry

comment:10 Changed 15 months ago by JeroenHoek

Description: modified (diff)

comment:11 Changed 15 months ago by JeroenHoek

I've removed the removal of the special treatment of footway=none from the patch.

comment:12 Changed 12 months ago by JeroenHoek

Just to clarify: this patch is no longer in conflict with any current developments. sidewalk=none is now considered deprecated in favour of sidewalk=no; this patch is limited to the items listed in the first post.

comment:13 Changed 9 months ago by skyper

Ticket #21912 has been marked as a duplicate of this ticket.

comment:14 Changed 9 months ago by skyper

Milestone: 22.03

comment:15 Changed 8 months ago by stoecker

Milestone: 22.0322.04

comment:16 Changed 7 months ago by stoecker

Milestone: 22.0422.05

Milestone renamed

comment:17 Changed 6 months ago by stoecker

Milestone: 22.0522.06

comment:18 Changed 5 months ago by taylor.smock

@skyper: Did you have any additional problems with the patch?

@JeroenHoek: My one problem is way[footway=none][/footway:/] { -> way[footway=none][/^footway:/] {. If this is correct, I'd like to see an assertMatch/assertNoMatch test.

Last edited 5 months ago by taylor.smock (previous) (diff)

comment:19 in reply to:  18 Changed 5 months ago by skyper

Replying to taylor.smock:

@skyper: Did you have any additional problems with the patch?

No, though, I did not test it with real data.

Changed 5 months ago by JeroenHoek

Patch

comment:20 Changed 5 months ago by JeroenHoek

@taylor.smock I just followed the example above to remain consistent. This seems to work.

comment:21 Changed 5 months ago by taylor.smock

I was thinking of tags like surface:footway (although footway:surface is way more popular). Although maybe it was for footway:left or footway:right. I'd have to dig through the history to figure out why there was a regex.

comment:22 Changed 5 months ago by stoecker

Milestone: 22.0622.07

comment:23 Changed 4 months ago by taylor.smock

Resolution: fixed
Status: newclosed

In 18523/josm:

Fix #21235: Stop whitelisting footway=*, and add footway=separate to blacklist candidates for sidewalk=* (patch by JeroenHoek)

Patch notes by JeroenHoek (modified):

  • Add a check for footway=separate.
  • Remove special treatment for footway=none, none is added to the general footway=* check.
  • The whitelist for footway=* is removed.

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.