-
Notifications
You must be signed in to change notification settings - Fork 1k
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
New vectortile layer for Digitransit realtime stops #5743
Changes from 5 commits
dbc00ee
c977425
2e201f7
c38e648
f80544b
dd4db60
816fba9
c148daf
796d542
5120e12
89e82e9
d75df7f
5132bf2
a408f01
e8f9488
b4a0f8e
e6c31e6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
package org.opentripplanner.ext.vectortiles.layers.stops; | ||
|
||
import static org.opentripplanner.ext.vectortiles.layers.stops.DigitransitStopPropertyMapper.getRoutes; | ||
import static org.opentripplanner.ext.vectortiles.layers.stops.DigitransitStopPropertyMapper.getType; | ||
|
||
import java.time.Instant; | ||
import java.time.ZonedDateTime; | ||
import java.util.Collection; | ||
import java.util.List; | ||
import java.util.Locale; | ||
import org.opentripplanner.apis.support.mapping.PropertyMapper; | ||
import org.opentripplanner.framework.i18n.I18NStringMapper; | ||
import org.opentripplanner.inspector.vector.KeyValue; | ||
import org.opentripplanner.routing.alertpatch.AlertEffect; | ||
import org.opentripplanner.transit.model.site.RegularStop; | ||
import org.opentripplanner.transit.service.TransitService; | ||
|
||
public class DigitransitRealtimeStopPropertyMapper extends PropertyMapper<RegularStop> { | ||
|
||
private final TransitService transitService; | ||
private final I18NStringMapper i18NStringMapper; | ||
|
||
public DigitransitRealtimeStopPropertyMapper(TransitService transitService, Locale locale) { | ||
this.transitService = transitService; | ||
this.i18NStringMapper = new I18NStringMapper(locale); | ||
} | ||
|
||
@Override | ||
protected Collection<KeyValue> map(RegularStop stop) { | ||
Instant currentTime = ZonedDateTime.now(transitService.getTimeZone()).toInstant(); | ||
boolean noServiceAlert = transitService | ||
.getTransitAlertService() | ||
.getStopAlerts(stop.getId()) | ||
.stream() | ||
.anyMatch(alert -> | ||
alert.effect().equals(AlertEffect.NO_SERVICE) && | ||
( | ||
alert.getEffectiveStartDate() != null && | ||
alert.getEffectiveStartDate().isBefore(currentTime) | ||
) && | ||
(alert.getEffectiveEndDate() != null && alert.getEffectiveEndDate().isAfter(currentTime)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you move this into a method on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also fixed the logic a little, as I realised that the end time doesn't need to be defined for the alert to be effective, it could be open ended. |
||
); | ||
|
||
return List.of( | ||
new KeyValue("gtfsId", stop.getId().toString()), | ||
new KeyValue("name", i18NStringMapper.mapNonnullToApi(stop.getName())), | ||
new KeyValue("code", stop.getCode()), | ||
new KeyValue("platform", stop.getPlatformCode()), | ||
new KeyValue("desc", i18NStringMapper.mapToApi(stop.getDescription())), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you extract a method for these You can define two lists and use |
||
new KeyValue( | ||
"parentStation", | ||
stop.getParentStation() != null ? stop.getParentStation().getId() : "null" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this "null" is a mistake we made in the old layer, you can just return |
||
), | ||
new KeyValue("type", getType(transitService, stop)), | ||
new KeyValue("routes", getRoutes(transitService, stop)), | ||
new KeyValue("noServiceAlert", noServiceAlert) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you want to keep the boolean, you could use a property name like "closedByServiceAlert". |
||
); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,7 @@ public class DigitransitStopPropertyMapper extends PropertyMapper<RegularStop> { | |
private final TransitService transitService; | ||
private final I18NStringMapper i18NStringMapper; | ||
|
||
private DigitransitStopPropertyMapper(TransitService transitService, Locale locale) { | ||
DigitransitStopPropertyMapper(TransitService transitService, Locale locale) { | ||
this.transitService = transitService; | ||
this.i18NStringMapper = new I18NStringMapper(locale); | ||
} | ||
|
@@ -34,20 +34,23 @@ protected static DigitransitStopPropertyMapper create( | |
|
||
@Override | ||
protected Collection<KeyValue> map(RegularStop stop) { | ||
Collection<TripPattern> patternsForStop = transitService.getPatternsForStop(stop); | ||
|
||
String type = patternsForStop | ||
.stream() | ||
.map(TripPattern::getMode) | ||
.collect(Collectors.groupingBy(Function.identity(), Collectors.counting())) | ||
.entrySet() | ||
.stream() | ||
.max(Map.Entry.comparingByValue()) | ||
.map(Map.Entry::getKey) | ||
.map(Enum::name) | ||
.orElse(null); | ||
return List.of( | ||
new KeyValue("gtfsId", stop.getId().toString()), | ||
new KeyValue("name", i18NStringMapper.mapNonnullToApi(stop.getName())), | ||
new KeyValue("code", stop.getCode()), | ||
new KeyValue("platform", stop.getPlatformCode()), | ||
new KeyValue("desc", i18NStringMapper.mapToApi(stop.getDescription())), | ||
new KeyValue( | ||
"parentStation", | ||
stop.getParentStation() != null ? stop.getParentStation().getId() : null | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a breaking change. This shouldn't be shared between the mappers as the old one always returned a string. |
||
), | ||
new KeyValue("type", getType(transitService, stop)), | ||
new KeyValue("routes", getRoutes(transitService, stop)) | ||
); | ||
} | ||
|
||
String routes = JSONArray.toJSONString( | ||
protected static String getRoutes(TransitService transitService, RegularStop stop) { | ||
return JSONArray.toJSONString( | ||
transitService | ||
.getRoutesForStop(stop) | ||
.stream() | ||
|
@@ -58,18 +61,20 @@ protected Collection<KeyValue> map(RegularStop stop) { | |
}) | ||
.toList() | ||
); | ||
return List.of( | ||
new KeyValue("gtfsId", stop.getId().toString()), | ||
new KeyValue("name", i18NStringMapper.mapNonnullToApi(stop.getName())), | ||
new KeyValue("code", stop.getCode()), | ||
new KeyValue("platform", stop.getPlatformCode()), | ||
new KeyValue("desc", i18NStringMapper.mapToApi(stop.getDescription())), | ||
new KeyValue( | ||
"parentStation", | ||
stop.getParentStation() != null ? stop.getParentStation().getId() : "null" | ||
), | ||
new KeyValue("type", type), | ||
new KeyValue("routes", routes) | ||
); | ||
} | ||
|
||
protected static String getType(TransitService transitService, RegularStop stop) { | ||
Collection<TripPattern> patternsForStop = transitService.getPatternsForStop(stop); | ||
|
||
return patternsForStop | ||
.stream() | ||
.map(TripPattern::getMode) | ||
.collect(Collectors.groupingBy(Function.identity(), Collectors.counting())) | ||
.entrySet() | ||
.stream() | ||
.max(Map.Entry.comparingByValue()) | ||
.map(Map.Entry::getKey) | ||
.map(Enum::name) | ||
.orElse(null); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for the detour via the ZonedDateTime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really? I thought the time zone might be important in this context to ensure the alert is actually valid according to local time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you call toInstant all timezone information is lost again. An instant is just a number of seconds since 1970-01-01 UTC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This instant can be converted to many time zones but it remains the same number of seconds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And since we only want to give an answer at the time of someone asking and not the future/past the time zone doesn't matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, you're right. Somehow tricked myself into thinking the instant is different in different places. 😂 Thanks!