Opening hours resurvey#6492
Conversation
|
Did you change the indentation? This makes it hard to see the changes in a review. |
Hard to avoid, I had to change the indentation level for most of the filter to adapt to the new logic. |
westnordost
left a comment
There was a problem hiding this comment.
I believe that now the getFeature field is unused for this class, right? Then, it should be removed.
class AddOpeningHours(
private val getFeature: (Element) -> Feature?
) : OsmElementQuestType<OpeningHoursAnswer>, AndroidQuest {
done! |
|
Right, thanks! LGTM |
| leisure ~ park|garden|beach_resort|sports_centre|disc_golf_course|nature_reserve|playground|fitness_station | ||
| or barrier | ||
| or tourism = attraction | ||
| or amenity ~ toilets|bicycle_rental|charging_station|place_of_worship|parking|research_institute|shower|grave_yard|kitchen|marketplace |
There was a problem hiding this comment.
is there any specific reason why amenity=grave_yard got added and landuse=cemetery is not present?
I am also thinking about extracting this set from megastring into another dictionary, though I admit that primary reason would be to make it parsable in my little tool
maybe that is a really bad reason for changing format here
There was a problem hiding this comment.
Not really, I just did not even consider cemetery (did not know there was a distinction). Extracting sounds good though
Based on the discussion in #6465.
It changes two things:
leisure = parkandamenity=charging_stationsince the other elements are already excluded from the name filter. This does not affect any of the other places that we add opening hours for.Thus we only ask for them if they already have (potentially out of date) opening_hours tagged.
These all seem like places that may have opening hours but commonly but also might not have them, so surveying them for elements where opening_hours is not set seems spammy.
All of these places seem to me like places that are unlikely to be confused, so a name is not required to correctly identify them because they are either: large enough so that they can be clearly identified even with inaccurate GPS or rare enough so that they are very unlikely to be close to each other.
3. I have removed a duplicate filter: We filter for name / brand twice, once in the filter and once with the
hasNamefunction. The former allowednoname = yeswhile the latter does not. This gets rid of code duplication and inconsistency. Instead we only use the filter.4. I added additional tests to verify the functionality.