Skip to content

Implement trip banning - #3953

Merged
hannesj merged 3 commits into
opentripplanner:dev-2.xfrom
entur:otp2_fix_trip_banning
Mar 8, 2022
Merged

Implement trip banning#3953
hannesj merged 3 commits into
opentripplanner:dev-2.xfrom
entur:otp2_fix_trip_banning

Conversation

@hannesj

@hannesj hannesj commented Mar 3, 2022

Copy link
Copy Markdown
Contributor

Summary

Trip banning was not implemented when moving from OTP1 to OTP2. This implements it, and fixes the descriptions in the Transmodel API for the unimplemented banned types.

Changelog

The changelog file
is generated from the pull-request title, make sure the title describe the feature or issue fixed.
To exclude the PR from the changelog add [changelog skip] in the title.

@hannesj hannesj added this to the 2.2 milestone Mar 3, 2022
@hannesj
hannesj requested a review from a team as a code owner March 3, 2022 13:37

@optionsome optionsome left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/src/main/java/org/opentripplanner/api/common/RoutingResource.java#L475 javadoc should be updated to reflect that it's now partly supported. Tested with REST API and seemd to work.

import org.opentripplanner.model.modes.AllowedTransitMode;
import org.opentripplanner.model.Trip;
import org.opentripplanner.routing.algorithm.raptoradapter.transit.TripPatternForDate;
import org.opentripplanner.routing.api.request.BannedStopSet;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This BannedStopSet is probably something we want to refactor out of at some point but seemingly the REST API allows to ban specific stops withing a trip

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm also not too keen on using an API class in the Raptor code.

@hannesj hannesj Mar 4, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is not in the RAPTOR code, it is in the raptor adapter, which sits between OTP and RAPTOR models. If you look below this class already uses two other classes from the same package, which will be moved when splitting up the REST API.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay.

@leonardehrenfried

Copy link
Copy Markdown
Member

Since the rest of the Raptor code is very well tested: how hard would it be to add a test?

@hannesj

hannesj commented Mar 4, 2022

Copy link
Copy Markdown
Contributor Author

Since the rest of the Raptor code is very well tested: how hard would it be to add a test?

Added

@leonardehrenfried leonardehrenfried left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you rewrite the Javadoc that @optionsome has pointed out?

@hannesj

hannesj commented Mar 7, 2022

Copy link
Copy Markdown
Contributor Author

Could you rewrite the Javadoc that @optionsome has pointed out?

Done

@hannesj
hannesj merged commit 29308b9 into opentripplanner:dev-2.x Mar 8, 2022
@hannesj
hannesj deleted the otp2_fix_trip_banning branch March 8, 2022 10:07
t2gran pushed a commit that referenced this pull request Mar 8, 2022
@t2gran t2gran added !Improvement A functional improvement or micro feature and removed Regression labels Sep 24, 2025
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants