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.

Sign up for GitHub

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

Jump to bottom

better deal with bad OSM data for OH #3719

Merged
merged 12 commits into from
Feb 11, 2022
Merged

better deal with bad OSM data for OH #3719

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
eae0c48
better deal with bad OSM data for OH
matkoniecz Feb 4, 2022
c6f0327
fix typo
matkoniecz Feb 4, 2022
af005e7
manually force Longs
matkoniecz Feb 5, 2022
2aff336
clean imports
matkoniecz Feb 5, 2022
8cebddf
Update app/src/main/java/de/westnordost/streetcomplete/quests/opening…
westnordost Feb 7, 2022
364a276
Update app/src/main/java/de/westnordost/streetcomplete/quests/opening…
westnordost Feb 7, 2022
c9962f3
Update app/src/main/java/de/westnordost/streetcomplete/quests/opening…
westnordost Feb 7, 2022
abac445
stop asking for recycling battery containers
matkoniecz Feb 9, 2022
1c0f639
Merge branch 'master' into OH
matkoniecz Feb 9, 2022
f9fed42
edit CheckOpeningHoursSigned filter to match AddOpeningHours filter
matkoniecz Feb 9, 2022
0d5794f
drop test that either require access to private function or overhelmi…
matkoniecz Feb 11, 2022
c4a64fc
Update app/src/main/java/de/westnordost/streetcomplete/quests/opening…
westnordost Feb 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 14 additions & 4 deletions app/src/main/java/de/westnordost/streetcomplete/quests/opening_hours/AddOpeningHours.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ class AddOpeningHours(
or amenity = parking and parking = multi-storey
or amenity = recycling and recycling_type = centre
or tourism = information and information = office
or (amenity = recycling and recycling:batteries = yes)
or """ +

// The common list is shared by the name quest, the opening hours quest and the wheelchair quest.
Expand Down Expand Up @@ -93,12 +92,23 @@ class AddOpeningHours(
),
).map { it.key + " ~ " + it.value.joinToString("|") }.joinToString("\n or ") + "\n" + """
)
and !opening_hours
and (!opening_hours or opening_hours older today -1 years)
)
or (
opening_hours older today -1 years
and (
leisure = park
or barrier
or amenity ~ toilets|bicycle_rental
)
)
or opening_hours older today -1 years
)
and access !~ private|no
and (name or brand or noname = yes or name:signed = no or amenity=recycling)
and (
name or brand or noname = yes or name:signed = no
or barrier
or amenity ~ toilets|bicycle_rental
)
and opening_hours:signed != no
""").toElementFilterExpression() }

Expand Down
5 changes: 4 additions & 1 deletion ...java/de/westnordost/streetcomplete/quests/opening_hours_signed/CheckOpeningHoursSigned.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,10 @@ class CheckOpeningHoursSigned (
or older today -1 years
)
and access !~ private|no
and (name or brand or noname = yes or name:signed = no or amenity = recycling)
and (
name or brand or noname = yes or name:signed = no
or amenity = recycling|toilets|bicycle_rental or leisure=park or barrier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we drop recycling from here too?

Also should this either be ~, or can the one in AddOpeningHours be = too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catches!

Fixed = vs ~ in 0848d98

amenity = recycling and recycling_type = centre should be either added to name quest or allowed to have no name in opening hours quest, not checked which would be preferable.

Or maybe in addition also removed from opening hours quest, is there distinction between recycling centres that allow walk ins and ones that are factories closed to typical people?

Copy link
Collaborator

Choose a reason for hiding this comment

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

amenity = recycling and recycling_type = centre should be either added to name quest or allowed to have no name in opening hours quest, not checked which would be preferable.

The recycling_type = centre bit isn't in the CheckOpeningHoursSigned code though.

I'm guessing you meant "rather than not checked"?

Or maybe in addition also removed from opening hours quest, is there distinction between recycling centres that allow walk ins and ones that are factories closed to typical people?

The recycling centres near me which allow walk/drive in from the public are normally council things which also accept waste too, would they be tagged like that not as something else? We used to call them a dump or tip, but they didn't used to recycle quite so much.

Copy link
Member Author

Choose a reason for hiding this comment

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

The recycling_type = centre bit isn't in the CheckOpeningHoursSigned code though.

Yes, but asking for extremely rare cases where this tag was manually placed on containers is also fine

Copy link
Member Author

Choose a reason for hiding this comment

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

https://wiki.openstreetmap.org/wiki/Tag:recycling_type%3Dcentre

a public recycling centre

So asking for opening hours is fine

)
""".toElementFilterExpression() }

private val hasOldOpeningHoursCheckDateFilter: String get() =
Expand Down
34 changes: 34 additions & 0 deletions app/src/test/java/de/westnordost/streetcomplete/quests/opening_hours/AddOpeningHoursTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import de.westnordost.streetcomplete.testutils.node
import org.junit.Assert.assertFalse
import org.junit.Assert.assertTrue
import org.junit.Test
import java.lang.System.currentTimeMillis
import java.time.LocalDate

class AddOpeningHoursTest {
Expand Down Expand Up @@ -160,6 +161,39 @@ class AddOpeningHoursTest {
)))
}

@Test fun `isApplicableTo returns false for known places with recently edited opening hours`() {
assertFalse(questType.isApplicableTo(
node(tags = mapOf("shop" to "sports", "name" to "Atze's Angelladen", "opening_hours" to "Mo-Fr 10:00-20:00"), timestamp = currentTimeMillis())
))
}

@Test fun `isApplicableTo returns true for known places with old opening hours`() {
val milisecondsFor400Days : Long = 1000L * 60 * 60 * 24 * 400
assertTrue(questType.isApplicableTo(
node(tags = mapOf("shop" to "sports", "name" to "Atze's Angelladen", "opening_hours" to "Mo-Fr 10:00-20:00"), timestamp = currentTimeMillis() - milisecondsFor400Days)
))
}

@Test fun `isApplicableTo returns false for closed shops with old opening hours`() {
val milisecondsFor400Days : Long = 1000L * 60 * 60 * 24 * 400
assertFalse(questType.isApplicableTo(
node(tags = mapOf("nonexisting:shop" to "sports", "name" to "Atze's Angelladen", "opening_hours" to "Mo-Fr 10:00-20:00"), timestamp = currentTimeMillis() - milisecondsFor400Days)
))
}

@Test fun `isApplicableTo returns true for parks with old opening hours`() {
val milisecondsFor400Days : Long = 1000L * 60 * 60 * 24 * 400
assertTrue(questType.isApplicableTo(
node(tags = mapOf("leisure" to "park", "name" to "Trolololo", "opening_hours" to "Mo-Fr 10:00-20:00"), timestamp = currentTimeMillis() - milisecondsFor400Days)
))
}

@Test fun `isApplicableTo returns false for toilets without opening hours`() {
assertFalse(questType.isApplicableTo(
node(tags = mapOf("amenity" to "toilets"), timestamp = currentTimeMillis())
))
}

@Test fun `isApplicableTo returns true if the opening hours cannot be parsed`() {
assertTrue(questType.isApplicableTo(node(
tags = mapOf(
Expand Down