opentripplanner / OpenTripPlanner

An open source multi-modal trip planner
http://www.opentripplanner.org
Other
2.12k stars 1.01k forks source link

Cluster consolidated stops in geocoder #5907

Open leonardehrenfried opened 3 weeks ago

leonardehrenfried commented 3 weeks ago

Summary

This connects two sandbox features with each other: IBI's consolidated stops and and the geocoder.

In order to do this, the LuceneIndex was moved out of the Graph and into a Dagger module which instantiates it (or not) based on the feature flags.

cc @miles-grant-ibigroup

Unit tests

I added a unit test.

Most importantly, it adds a dependency on the assertion library truth by Google which allows you to write assertions against collections (among other things) which have very helpful error messages, like this:

value of           : generateStopClusters(...)
expected to contain: LuceneStopCluster[primaryId=F:A, secondaryIds=[F:B], names=[A, B], codes=[A], coordinate=Coordinate[lat=60.0, lon=10.0]]
but was            : [LuceneStopCluster[primaryId=F:C, secondaryIds=[], names=[C], codes=[C], coordinate=Coordinate[lat=60.0, lon=10.0]], LuceneStopCluster[primaryId=F:A, secondaryIds=[F:B], names=[A, B], codes=[A, B], coordinate=Coordinate[lat=60.0, lon=10.0]]]
codecov[bot] commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 75.47170% with 13 lines in your changes missing coverage. Please review.

Project coverage is 69.47%. Comparing base (b9e54d2) to head (9b17f97). Report is 42 commits behind head on dev-2.x.

:exclamation: Current head 9b17f97 differs from pull request most recent head f95fd60

Please upload reports for the commit f95fd60 to get more accurate results.

Files Patch % Lines
...opentripplanner/ext/geocoder/GeocoderResource.java 0.00% 4 Missing :warning:
...planner/ext/geocoder/configure/GeocoderModule.java 0.00% 4 Missing :warning:
.../org/opentripplanner/ext/geocoder/LuceneIndex.java 60.00% 2 Missing :warning:
...ner/standalone/configure/ConstructApplication.java 0.00% 2 Missing :warning:
...standalone/server/DefaultServerRequestContext.java 50.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev-2.x #5907 +/- ## ============================================= + Coverage 69.45% 69.47% +0.01% - Complexity 17064 17082 +18 ============================================= Files 1927 1932 +5 Lines 73578 73615 +37 Branches 7549 7550 +1 ============================================= + Hits 51106 51146 +40 + Misses 19847 19845 -2 + Partials 2625 2624 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

t2gran commented 3 weeks ago

I looked at Truth vs AssertJ. They are similar. AssertJ is a bit more open, but Truth is probably a bit more well designed.

This is written by the people creating Truth, but seems to be fairly objective and the best comparison I could find. I also looked at a few key numbers on the github repo (stars/users/activity) and they both seems to be maintained and mature.

Conclusion: I like Truth slightly better than AssertJ. I support starting to experiment with Truth (the core lib only) and decide later if we want to keep it or not. This do add "another lib" witch a OTP developer should master. There are pitfalls, so it everyone doing reviews need to spend time on learning it, if we decide to go "all in".

leonardehrenfried commented 2 weeks ago

Note to self: add comment to LuceneIndex constructor.