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

Survey incomplete sidewalk tagging #3821

Merged

Conversation

arrival-spring
Copy link
Contributor

Some places only have sidewalk:left or sidewalk:right. I think this will usually imply that the other side has none, but I think it still counts as incomplete data (and would block #3735 - sidewalk surface quest).

This adds a bit more complexity than I'd hoped to the sidewalk filter, I wanted to do
and !sidewalk and !sidewalk:both and !(sidewalk:left and sidewalk:right)
but the NOT operator doesn't seem to work with brackets (it complained I'd closed too many brackets).

I wasn't about to get into rewriting the filter code, and maybe this filter is more readable than the above anyway.

Tested and works as expected, and all tests pass.

(I don't think this will have merge issues with #3735 but I'm happy to sort them if it does)

@westnordost
Copy link
Member

and !sidewalk and !sidewalk:both and !(sidewalk:left and sidewalk:right)

Would be wrong anyway. That filter would always return true if e.g. only sidewalk:both but not any other sidewalk:* was set

@arrival-spring
Copy link
Contributor Author

Now branched from #3735 - sidewalk surface quest

Thanks for the tip, I've now done the filters here in a similar way to AddCycleway, so any incomplete or overloaded tagging is now surveyed every time.

Runs as expected, and all tests pass.

@westnordost
Copy link
Member

Something is wrong, the diff also includes changes from your other PR even though it is merged now

@arrival-spring arrival-spring changed the base branch from master to 40 March 4, 2022 17:11
@arrival-spring arrival-spring changed the base branch from 40 to master March 4, 2022 17:11
@arrival-spring
Copy link
Contributor Author

Something is wrong, the diff also includes changes from your other PR even though it is merged now

It seems that's a bug in Github

I've sorted it now

Copy link
Member

@westnordost westnordost left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you! I find it very important though to have a test for SidewalkParser

@arrival-spring
Copy link
Contributor Author

Tests added (and fixed a couple of edge cases whilst at it).

When using only one of sidewalk:left and sidewalk:right SidewalkParser currently returns INVALID for the other side (rather than null).

I'm not sure if this matters, I didn't change it because it would mean adding a check for any side being null in AddSidewalk (in addition to checking for any side being INVALID.

@westnordost
Copy link
Member

Cool, you even found some bugs with that! The tests are really easy to read, well done!

When using only one of sidewalk:left and sidewalk:right SidewalkParser currently returns INVALID for the other side (rather than null).

Hmm, I think this is fair. It would be OK either way. The important part is that the app asks the user to complete the information if it is incomplete / ambiguous this way. So, whatever you think is best.

@westnordost westnordost merged commit ec0c991 into streetcomplete:master Mar 6, 2022
@arrival-spring arrival-spring deleted the incomplete-sidewalk branch March 6, 2022 22:49
@mnalis mnalis mentioned this pull request Mar 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants