#6373 Ask for ferry relationships and ferry ways not part of a ferry relationship#6375
Conversation
paulklie
changed the title
|
Big thanks for implementing it!
it may be worth adding also an automated tests see say https://github.com/streetcomplete/StreetComplete/blob/2c4303539600c176c8adcb7d1b831edad128496d/app/src/androidUnitTest/kotlin/de/westnordost/streetcomplete/quests/building_entrance/AddEntranceTest.kt or https://github.com/streetcomplete/StreetComplete/blob/2c4303539600c176c8adcb7d1b831edad128496d/app/src/androidUnitTest/kotlin/de/westnordost/streetcomplete/quests/address/AddHousenumberTest.kt as a base |
Added tests. |
|
For testing, I can recommend this place: It has all combinations: a simple ferry route that is just a way, and a way that is part of a relation right next to eachother. |
There was a problem hiding this comment.
@matkoniecz is correct, this is the wrong point at which to make the ignored relation types more dynamic/flexible because which relations are ignored is already relevant for the Uploader.
The correct point would be to replace val IGNORED_RELATION_TYPES: Set<String> in ApplicationConstants.kt with a function like
fun isRelationIgnored(tags: Map<String, String>): Boolean
This function is then passed as a parameter to all places where currently IGNORED_RELATION_TYPES is passed to and those places need to be refactored to accept a function. (For example, maybe there are other solutions, too)
This part, by the way, should ideally be a separate PR, enabling this PR.
Regarding FerryUtils.kt, it looks fine. More consistent with the rest of the code and probably more performant however would be a more functional style, e.g. something like
val ferryRouteWays: Collection<Way> = ...
val ferryRouteRelations: Collection<Relation> = ...
val wayIdsInFerryRouteRelations: Set<Long> = ...
return ferryRouteRelations + ferryRouteWays.filter { it.id !in wayIdsInFerryRouteRelations }(More performant because currently you have to iterate through all relations for every single ferry route way)
This is pretty compact and straightforward, little duplicated code, so maybe FerryUtils is not even necessary anymore.
I am afraid this exceeds my limited kotlin knowledge, and I dont know how todo this.
Hmm I think we can also reuse it for other ferry quests (MaxWeight, Fee) as well as further ones that might be useful (Opening Hours, Maxweight?) to. |
|
Well, for the record, I don't like it, but it seems to be common tagging practice, so it unfortunately makes sense for StreetComplete to take this into account. In my last commit, I implemented what I noted in my last comment. I kept your utils file and changed the tests to test exactly only that helper function. |
Ok great thanks! |
3 participants
resolve #6373
I don't have much experience in kotlin, or this repo so am unsure if I conforms to coding guidelines, so some feedback would be good. But it seems to work with my testing.
Since we might be adding many ferry quests I think it might make sence to move some duplicate code to a single place, but not sure where that should be.