Skip to content

Add feature switch for matching ET messages on stops - #4347

Merged
hannesj merged 1 commit into
opentripplanner:dev-2.xfrom
Skanetrafiken:17632_match_on_stops
Sep 20, 2022
Merged

Add feature switch for matching ET messages on stops#4347
hannesj merged 1 commit into
opentripplanner:dev-2.xfrom
Skanetrafiken:17632_match_on_stops

Conversation

@Bartosz-Kruba

Copy link
Copy Markdown
Contributor

Summary

Add feature switch for matching ET messages on stops

Issue

closes #4346

Unit tests

No new unit tests added

Documentation

Updated documentation

@Bartosz-Kruba
Bartosz-Kruba requested a review from a team as a code owner August 4, 2022 12:41
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

Merging #4347 (2b13dc5) into dev-2.x (ee0859b) will increase coverage by 0.00%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             dev-2.x    #4347   +/-   ##
==========================================
  Coverage      57.54%   57.54%           
  Complexity     10933    10933           
==========================================
  Files           1441     1441           
  Lines          58255    58256    +1     
  Branches        6772     6773    +1     
==========================================
+ Hits           33520    33521    +1     
  Misses         22717    22717           
  Partials        2018     2018           
Impacted Files Coverage Δ
...pplanner/ext/siri/SiriTimetableSnapshotSource.java 0.00% <0.00%> (ø)
...main/java/org/opentripplanner/util/OTPFeature.java 85.45% <100.00%> (+0.26%) ⬆️
...ripplanner/model/transfer/ConstrainedTransfer.java 80.00% <0.00%> (-3.34%) ⬇️
...pplanner/graph_builder/module/osm/OSMDatabase.java 76.47% <0.00%> (+0.18%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us.

@hannesj

hannesj commented Aug 7, 2022

Copy link
Copy Markdown
Contributor

I think it would be better with an updater parameter, rather than a feature flag.
Example from SIRI-VM: https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/src/main/java/org/opentripplanner/standalone/config/updaters/SiriVMUpdaterConfig.java#L16

@t2gran t2gran added this to the 2.2 milestone Aug 11, 2022
@t2gran t2gran added the !Improvement A functional improvement or micro feature label Aug 11, 2022
@t2gran

t2gran commented Aug 11, 2022

Copy link
Copy Markdown
Member

@Bartosz-Kruba We discussed this at the developer meeting, so if you do the switch to config - not OTPFeature as @hannesj suggest, then this looks ok.

hannesj
hannesj previously approved these changes Aug 16, 2022
@hannesj

hannesj commented Aug 21, 2022

Copy link
Copy Markdown
Contributor

Is there some reason the parameter is not called fuzzyTripMatching as with SIRI-VM and GTFS-RT updaters? I think it would bee good to have consistency on these

@leonardehrenfried

Copy link
Copy Markdown
Member

You want to run the formatter on your code.

@MikaelJarfors MikaelJarfors added the Skanetrafiken On skanetrafikens roadmap label Aug 29, 2022
@Bartosz-Kruba
Bartosz-Kruba force-pushed the 17632_match_on_stops branch 2 times, most recently from d8c270f to 41da116 Compare September 6, 2022 14:08
@Bartosz-Kruba

Copy link
Copy Markdown
Contributor Author

I've changed the name of config parameter name and did a rebase. Had to go for other solution since SiriTimetableSnapshotSource is no longer instantiated the same way as it was before.

hannesj
hannesj previously approved these changes Sep 8, 2022
@hannesj
hannesj requested a review from lassetyr September 12, 2022 08:29

@lassetyr lassetyr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this approach, the updater still instantiates the SiriFuzzyTripMatcher, and passes it to the SiriTimetableSnapshotSource.

Would it be easier to handle this flag only in the updater, and set SiriFuzzyTripMatcher = null when this feature is switched off?

SiriTimetableSnapshotSource would then only need a null-check.

@Bartosz-Kruba

Bartosz-Kruba commented Sep 20, 2022

Copy link
Copy Markdown
Contributor Author

With this approach, the updater still instantiates the SiriFuzzyTripMatcher, and passes it to the SiriTimetableSnapshotSource.

Would it be easier to handle this flag only in the updater, and set SiriFuzzyTripMatcher = null when this feature is switched off?

SiriTimetableSnapshotSource would then only need a null-check.

This is not so easy since SiriFuzzyTripMatcher is used for other things as well. It would require us to move around a lot of code. I would rather not do that since we are planning to refactor whole module anyway

@hannesj
hannesj merged commit 5813e2f into opentripplanner:dev-2.x Sep 20, 2022
t2gran pushed a commit that referenced this pull request Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

!Improvement A functional improvement or micro feature Skanetrafiken On skanetrafikens roadmap

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SIRI module: make it is possible to switch off matching trips on stops

7 participants