Handle duration tags for elevators in the same way as for escalators #6568
Conversation
tkalvas
added
Improvement
leonardehrenfried
changed the title
optionsome
removed
the
bump serialization id
| var from = edge.getFromVertex(); | ||
| var req = StreetSearchRequest.of().withMode(StreetMode.WALK); | ||
| var res = edge.traverse(new State(from, req.build()))[0]; | ||
| assertEquals(62_000, res.getTimeDeltaMilliseconds()); |
There was a problem hiding this comment.
This PR is fine as it is but where do you stand about allowing getters just for tests instead of testing the traversal logic in the same test?
If you are fine with it, this can be split in two tests, but it's not a requirement for my approval.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #6568 +/- ##
=============================================
+ Coverage 70.25% 70.29% +0.04%
- Complexity 18388 18393 +5
=============================================
Files 2088 2088
Lines 77419 77419
Branches 7842 7840 -2
=============================================
+ Hits 54387 54419 +32
+ Misses 20263 20229 -34
- Partials 2769 2771 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
This looks already pretty great. However, I had a few requests about the tests but rather than playing review ping pong I chose to make the changes myself.
If you agree with these, can you cherry pick the commit into this branch. Then this is good to merge IMO.
Summary
Elevators can now also have a hh:mm:ss format duration tag in OSM, and we recognize them.
Issue
The way we handled elevator duration tags was against OSM data spec.
Unit tests
The OsmProcessors are hard to test, and because of that are undertested. I added one here.
Changelog
Yes.
Bumping the serialization version id
Yes.