Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename bike rental vehicle rental in internal classes #3595

Merged

Conversation

hannesj
Copy link
Contributor

@hannesj hannesj commented Sep 1, 2021

Summary

This PR renames bike rental to vehicle rental in internal classes. This change is meant to reduce the mental overhead, as the classes support other types of vehicles, like cars and micromobility vehicles. It is based on top of #3562 and #3568.

  • Rename classes to VehicleRental
  • Rename configuration inputs, with old keys as backup for backwards compatibility
  • Update configuration examples to match current behaviour and remove references to old updaters
  • Split VehicleRentalStation into internal model and API classes for backwards compatibility in the API
  • Remove custom equals and hashCode from VehicleRentalStation

Issue

Relates to #2778 and MobilityData/gbfs#354

Unit tests

Pure refactoring, existing tests pass

Documentation

  • Updated documentation to match new behaviour

Changelog

Was a bullet point added to the changelog file with description and link to the linked issue?

@hannesj hannesj requested a review from a team as a code owner September 1, 2021 07:18
@leonardehrenfried
Copy link
Member

I tried this PR with the data from our region and I can confirm that it still works.

Screenshot from 2021-09-02 11-56-06

@t2gran t2gran added this to the 2.1 milestone Sep 2, 2021
@@ -42,7 +42,7 @@
- Do not allow bicycle traversal on ways tagged with mtb:scale [#3578](https://github.com/opentripplanner/OpenTripPlanner/pull/3578)
- Changes to the StopTimes call [#3576](https://github.com/opentripplanner/OpenTripPlanner/issues/3576)
- Fix bug in optimize transfer service decorating path [#3587](https://github.com/opentripplanner/OpenTripPlanner/issues/3587)

- Remove non-GBFS bicycle rental updaters [#3562](https://github.com/opentripplanner/OpenTripPlanner/issues/3562)
Copy link
Member

Choose a reason for hiding this comment

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

FYI, we usually try to have 2 blank lines above a header and 1 blank line below. No need to fix this, we will insert an extra line next time around.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needed fixing anyway due to merge conflict

@@ -1,35 +1,37 @@
package org.opentripplanner.routing.bike_rental;
package org.opentripplanner.routing.vehicle_rental;
Copy link
Member

Choose a reason for hiding this comment

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

Strictly using a _ in the package name is not recommended, but I think it is much more readable than not. We should probably make an exception for this in the coding guideline. I am in favor of keeping the ´_´ here.

t2gran
t2gran previously approved these changes Sep 2, 2021
Copy link
Member

@t2gran t2gran left a comment

Choose a reason for hiding this comment

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

I think this looks ok. It is a lot of changes, but I think the way forward is the merge this and fix any error reported.

@t2gran
Copy link
Member

t2gran commented Sep 2, 2021

We will wait for @leonardehrenfried to do a smoke test on this before merging, just to be safe.

@hannesj
Copy link
Contributor Author

hannesj commented Sep 2, 2021

We will wait for @leonardehrenfried to do a smoke test on this before merging, just to be safe.

I think this was already confirmed here #3595

@leonardehrenfried
Copy link
Member

Yes, I confirmed that they work upthread.

@hannesj What you see now is the snapshot tests failing. You want to delete the snapshot files and rerun the tests to regenerate them. After that you can check the JSON for unexpected changes.

Note that the snapshot tests compare the internal model, not API classes, which were unchanged.
…ntur/OpenTripPlanner into otp2_bike_rental_vehicle_rental_rename
@hannesj hannesj merged commit 35c6dde into opentripplanner:dev-2.x Sep 2, 2021
@hannesj hannesj deleted the otp2_bike_rental_vehicle_rental_rename branch September 2, 2021 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants