Filter import of rental data by pickup type

[sharhio]

Summary

Often only station data or only vehicle data is of interest to our use cases. In those cases the other type of data being read in is unnecessary, inefficient and also problematic, as we get places and routes that are of no value and have to be filtered out on the UI side anyway.

I've added a parameter to the updater configuration, which determines what type of vehicle rental data is read in (stations/vehicles/all).

Unit tests

• Updated related tests with the new parameter value.
• Manually tested.

Documentation

• Updater config documentation updated.

[codecov]

Codecov Report

Attention: Patch coverage is 86.36364% with 6 lines in your changes missing coverage. Please review.

Project coverage is 69.71%. Comparing base (5b5d92f) to head (7735de7).
Report is 17 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...ental/datasources/GbfsVehicleRentalDataSource.java	84.21%	0 Missing and 3 partials ⚠️
...g/updaters/sources/VehicleRentalSourceFactory.java	80.00%	2 Missing ⚠️
...kerental/SmooveBikeRentalDataSourceParameters.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             dev-2.x    #6240   +/-   ##
==========================================
  Coverage      69.71%   69.71%           
- Complexity     17696    17702    +6     
==========================================
  Files           2008     2009    +1     
  Lines          75834    75857   +23     
  Branches        7765     7766    +1     
==========================================
+ Hits           52866    52886   +20     
- Misses         20256    20258    +2     
- Partials        2712     2713    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

---

🚨 Try these New Features:

• JS Bundle Analysis - Avoid shipping oversized bundles

[optionsome]

Clarify that this parameter is only temporary and it's going to be removed.

[optionsome]

You could extract this code to be a method inside the params.

[optionsome]

Same as above and you could do the OTPFeature.FloatingBike.isOn() check inside the same if.

[optionsome]

This class should be inside org.opentripplanner.updater.vehicle_rental.datasources.params package.

[leonardehrenfried]

Please implement DocumentedEnum which makes it trivial to keep documentation up to date. You can use it like this:

OpenTripPlanner/application/src/main/java/org/opentripplanner/standalone/config/routerequest/ItineraryFiltersConfig.java

Lines 70 to 77 in f4bfedf

	.withDebug(
	c
	.of("debug")
	.since(V2_0)
	.summary(ItineraryFilterDebugProfile.OFF.typeDescription())
	.description(docEnumValueList(ItineraryFilterDebugProfile.values()))
	.asEnum(dft.debug())
	)

[leonardehrenfried]

Please write a constructor that makes sure this is never null. Like this:

OpenTripPlanner/application/src/main/java/org/opentripplanner/routing/alertpatch/AlertUrl.java

Lines 7 to 9 in f492446

	public AlertUrl {
	Objects.requireNonNull(uri);
	}

[optionsome]

This should still be included in some way, probably replace the "Temporary parameter" in the sentence above this with this content..

[t2gran]

@optionsome / @leonardehrenfried What is the right name for this?

I would not include "Allowed" in the name, it restrict the type, so it can not be reused.

[leonardehrenfried]

It's the "type" (?) of the rental: either free-floating or station-based.

[leonardehrenfried]

We discussed that we want to use a Set of opt-in rental types. I've used the same pattern when filtering what to import from NeTEx:

OpenTripPlanner/application/src/main/java/org/opentripplanner/netex/loader/parser/NetexDocumentParser.java

Lines 82 to 84 in f4bfedf

	} else if (!ignoredFeatures.contains(FARE_FRAME) && value instanceof FareFrame) {
	parse((FareFrame) value, new FareFrameParser());
	} else if (value instanceof CompositeFrame) {

In our case we probably want to use opt-in rather than opt-out as it's easier to reason about.

[t2gran]

This class should be renamed to RentalPickupType and have {STATION, FREE_FLOTING} as values, not ALL.

@leonardehrenfried do you agree? I changed the VEHICLES to FREE_FLOTING. Also use singular, not plural in enums.

In places where there is a set of these you should use Set<RentalPickupType>. I would define a constant in the enum class for ALL like this:

public static final Set<RentalPickupType> ALL = Collections.unmodifiableSet(EnumSet.allOf(RentalPickupType.class));

The unmodifiableSet(...) is needed to avoid modification and you should use it instead of Set.copyOf() (witch convert the internal set to a HashSet).

[leonardehrenfried]

I agree.

[leonardehrenfried]

I have a small request about the documentation.

[optionsome]

I'm not sure if I like the enum name or its documentation as it's quite use case specific but the enum is seemingly generic. However, I think we will get rid of the enum soon as well.