Modify

Opened 3 years ago

Closed 20 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 21 months ago.
Patch

Download all attachments as: .zip

Change History (24)

comment:1 by JeroenHoek, 3 years ago

Description: modified (diff)

comment:2 by JeroenHoek, 3 years ago

Component: CoreCore validator
Type: defectenhancement

comment:3 by JeroenHoek, 3 years ago

Description: modified (diff)

comment:4 by skyper, 3 years ago

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

comment:5 by skyper, 3 years ago

Keywords: footway sidewalk added

comment:6 by anonymous, 3 years ago

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 by JeroenHoek, 3 years ago

(Comment by me.)

comment:9 by mkoniecz, 3 years ago

that was me, sorry

comment:10 by JeroenHoek, 3 years ago

Description: modified (diff)

comment:11 by JeroenHoek, 3 years ago

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

comment:12 by JeroenHoek, 2 years ago

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 by skyper, 2 years ago

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

comment:14 by skyper, 2 years ago

Milestone: 22.03

comment:15 by stoecker, 2 years ago

Milestone: 22.0322.04

comment:16 by stoecker, 22 months ago

Milestone: 22.0422.05

Milestone renamed

comment:17 by stoecker, 21 months ago

Milestone: 22.0522.06

comment:18 by taylor.smock, 21 months ago

@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 21 months ago by taylor.smock (previous) (diff)

in reply to:  18 comment:19 by skyper, 21 months ago

Replying to taylor.smock:

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

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

comment:20 by JeroenHoek, 21 months ago

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

comment:21 by taylor.smock, 21 months ago

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 by stoecker, 20 months ago

Milestone: 22.0622.07

comment:23 by taylor.smock, 20 months ago

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. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.