Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add duplicate way validation #220

Merged
merged 5 commits into from
Jun 14, 2021

Conversation

Bonkles
Copy link
Contributor

@Bonkles Bonkles commented May 19, 2021

This check adds a duplicate way validation to RapiD, by looking for single-segment ways that share the same two endpoint locations. If such segments are found, we highlight the endpoints in a validation entry.

// Consider a way to be routable if it is a highway, railway, or wateray.
// if it is an area of any kind, it is not routable.
function hasRoutableTags(way) {
if (Object.keys(way.tags).some(isAreaTag)) return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking whether a geometry is an area is a bit more complicated than just looking for an area tag...
Way has a builtin method way.isArea() to do it.



function getIssuesForWay(way) {
var issues = [],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we stick the hasRoutableTags check here at the very beginning? I'm thinking it could avoid looping through all the nodes and make this code much more performant..

@bhousel
Copy link
Contributor

bhousel commented May 24, 2021

Looks good! Another thing I was thinking (aside from the above code comments) was that #212 had requested validations for a bunch of duplicate way issues. This PR just handles a subset of the issues, but can we expand the code later to catch more? I think what I'm asking is if the code is organized in a way that in the future we can add more of these kinds of ValidationIssue to the duplicate_way_segments.js file, and avoid creating more files with more checks. (I think the answer is "yes" but I'm not sure).

@Bonkles
Copy link
Contributor Author

Bonkles commented Jun 3, 2021

Looks good! Another thing I was thinking (aside from the above code comments) was that #212 had requested validations for a bunch of duplicate way issues. This PR just handles a subset of the issues, but can we expand the code later to catch more...

I'll ask @atiannicelli what he thinks about whether this check should get more involved before we add it.

@atiannicelli
Copy link

Any incremental changes we can make to get closer to the desired state I am fine with. If this doesn't solve all the parts of that issue then I'll split out the things it doesn't into another issue so we can close one and keep the other around.

@Bonkles
Copy link
Contributor Author

Bonkles commented Jun 3, 2021

Sounds good- I'll address Bryan's first couple of comments and then get this updated, then we can ship, then re-do the task, and iterate!

Copy link
Contributor Author

@Bonkles Bonkles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix function spacing!

modules/validations/duplicate_way_segments.js Show resolved Hide resolved
@Bonkles Bonkles marked this pull request as ready for review June 11, 2021 14:56
@Bonkles
Copy link
Contributor Author

Bonkles commented Jun 11, 2021

@bhousel doink! I have promoted this PR to 'ready for review'.

@bhousel
Copy link
Contributor

bhousel commented Jun 14, 2021

Looks great! Thanks @Bonkles 🎉

@bhousel bhousel merged commit b29d8f6 into facebook:main Jun 14, 2021
@bhousel bhousel added this to the v.next milestone Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants