Improve cleanup of old parking tags #4549
Conversation
|
Very nice, you even added all those new test cases! Will review asap |
… fit that, no code changes otherwise)
app/src/test/java/de/westnordost/streetcomplete/osm/street_parking/StreetParkingKtTest.kt
Show resolved
Hide resolved
app/src/main/java/de/westnordost/streetcomplete/osm/street_parking/StreetParking.kt
Outdated
Show resolved
Hide resolved
| if (right == left) { | ||
| add(Regex("^parking:lane:.*")) | ||
| if (conditionLeft != null && conditionRight == conditionLeft) { | ||
| add(Regex("^parking:condition:.*")) | ||
| } |
There was a problem hiding this comment.
Huh? Everything is removed when left and right are the same? And something with conditions?
I don't quite understand this, shouldn't this if-else-block be about checking whether each side changed only?
There was a problem hiding this comment.
The else was too much due to my incorrect test case.
I added this code to cover the issue reported in #4534. Previously, the parking:lane:side tags of the already specified side were not removed correctly if the user were to specify the same parking situation on the other side.
To fix the same problem for parking:condition:side, this code cleans up any preexisting parking:condition tags when the new parking situation of both sides is the either no_parking, no_standing, and no_stopping (i.e. one of the parking conditions currently supported by StreetComplete). However, this code line could probably be changed so that really only the previous parking:condition:side tag is removed and no other subtags of the unchanged side.
- allow parsing only position or orientation without defaulting to "incomplete" - mark each position and orientation as unknown individually, not for the whole side
|
I added more granular parsing to enable implementation of the suggestions made here #4501 (comment) They write that StreetComplete should also not delete parking tags for the side that is changed if it is merely supplemented to be complete tagging rather than changing anything. |
|
I'm currently looking into the code for that and maybe changing something. |
|
Done for now... (there are failing tests - that are not my fault though). Though... the crazy complex implementation (at least when one regards that all those edge cases need tests) makes me start to wonder if it is really necessary / correct to clear all those subtags if the parking position and orientation changed. The argument has been that if that changed, likely the street (parking) was redesigned, so anything parking related could have changed. But with this argumentation, we would also need to remove |
|
Should I still try to fix my implementation? Only the test "applying only one side removes properties specified for both sides" fails currently. What does this test case cover anyway? Is it even possible to delete an already existing side using the app? |
No |
|
Hum, I spotted some other issue while adding some tests but I forgot what it was. It was probably some edge case to the "do clear the tags if something changed". But anyway, I guess I should revert/redo some of all this to account for #4549 (comment) ... or at least think a bit more about this first. |
|
With the changes in my last commit, all test cases are currently passed. I'm in the middle of trying to answer the review comment that's still open. However, if the implementation is to be changed completely, I could skip that. |
|
Hmm, I thought about it some more and I think it's better to not clear the whole |
|
But this PR now contains so much stuff for the conditional removal of subtags that I better start fresh and only selectively copy over some things from this PR |
Closes #4534.
I have not seen any problems while testing and the new test cases all work as well. However, I am not really sure if the new code covers all possible parking situations and user selections.