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

Support for Route longName translations #4356

Merged
merged 16 commits into from
Aug 26, 2022

Conversation

Antiik91
Copy link
Contributor

@Antiik91 Antiik91 commented Aug 8, 2022

Summary

This PR will adds translation to Route's longName property.

Issue

This PR relates to issue #2832

Unit tests

Unit tests have been updated where necessary.

Documentation

Documentation hasn't been updated in this PR.

@optionsome optionsome added Skip Changelog bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR Improvement GTFS Related to import of GTFS data labels Aug 8, 2022
…GraphQLQueryTimeImpl so it suits better for translated strings for route longName property
@Antiik91 Antiik91 marked this pull request as ready for review August 10, 2022 11:30
@Antiik91 Antiik91 requested a review from a team as a code owner August 10, 2022 11:30
…st accordingly. Removed nullcheker for translationHelper, as it is not needed anymore
@codecov-commenter
Copy link

codecov-commenter commented Aug 23, 2022

Codecov Report

Merging #4356 (b7d3fdb) into dev-2.x (0ead418) will increase coverage by 0.14%.
The diff coverage is 65.14%.

@@              Coverage Diff              @@
##             dev-2.x    #4356      +/-   ##
=============================================
+ Coverage      57.93%   58.08%   +0.14%     
- Complexity     11004    11071      +67     
=============================================
  Files           1450     1455       +5     
  Lines          58690    58776      +86     
  Branches        6758     6770      +12     
=============================================
+ Hits           34004    34139     +135     
+ Misses         22644    22599      -45     
+ Partials        2042     2038       -4     
Impacted Files Coverage Δ
.../fares/impl/TimeBasedVehicleRentalFareService.java 0.00% <0.00%> (ø)
...opentripplanner/ext/fares/model/FareAttribute.java 63.63% <ø> (ø)
.../org/opentripplanner/ext/fares/model/FareRule.java 69.56% <0.00%> (ø)
...org/opentripplanner/ext/flex/FlexAccessEgress.java 90.90% <ø> (ø)
...t/java/org/opentripplanner/ext/flex/FlexIndex.java 76.92% <0.00%> (ø)
...g/opentripplanner/ext/flex/FlexibleTransitLeg.java 42.85% <0.00%> (ø)
...pplanner/ext/flex/template/FlexAccessTemplate.java 65.21% <0.00%> (ø)
...pplanner/ext/flex/template/FlexEgressTemplate.java 64.70% <0.00%> (ø)
...va/org/opentripplanner/ext/flex/trip/FlexTrip.java 71.42% <0.00%> (ø)
...opentripplanner/ext/flex/trip/UnscheduledTrip.java 49.49% <0.00%> (ø)
... and 221 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

hannesj
hannesj previously approved these changes Aug 26, 2022
Comment on lines 912 to 923
routeStream.filter(route -> {
if (
route.getShortName() != null &&
LegacyGraphQLUtils.startsWith(route.getShortName(), name, environment.getLocale())
) {
return true;
}
return (
route.getLongName() != null &&
LegacyGraphQLUtils.startsWith(route.getLongName(), name, environment.getLocale())
);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

If we add a null check in LegacyGraphQLUtils.startsWith, this could be simplified down to

Suggested change
routeStream.filter(route -> {
if (
route.getShortName() != null &&
LegacyGraphQLUtils.startsWith(route.getShortName(), name, environment.getLocale())
) {
return true;
}
return (
route.getLongName() != null &&
LegacyGraphQLUtils.startsWith(route.getLongName(), name, environment.getLocale())
);
});
routeStream.filter(route ->
LegacyGraphQLUtils.startsWith(route.getShortName(), name, environment.getLocale()) ||
LegacyGraphQLUtils.startsWith(route.getLongName(), name, environment.getLocale())
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@leonardehrenfried leonardehrenfried left a comment

Choose a reason for hiding this comment

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

I like how concise the change is in the graphql API.

@optionsome optionsome merged commit 0efea3b into opentripplanner:dev-2.x Aug 26, 2022
@optionsome optionsome deleted the DT-5398 branch August 26, 2022 12:03
t2gran pushed a commit that referenced this pull request Aug 26, 2022
@t2gran t2gran added this to the 2.2 milestone Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR GTFS Related to import of GTFS data Improvement Skip Changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants