Skip to content

Read translations from GTFS and Netex sources for stop names - #3808

Merged
hannesj merged 28 commits into
opentripplanner:dev-2.xfrom
entur:otp2_read_netex_translations
Mar 17, 2022
Merged

Read translations from GTFS and Netex sources for stop names#3808
hannesj merged 28 commits into
opentripplanner:dev-2.xfrom
entur:otp2_read_netex_translations

Conversation

@hannesj

@hannesj hannesj commented Jan 10, 2022

Copy link
Copy Markdown
Contributor

Summary

Read translations for stop names from Netex feeds and expose them through the Transmodel API.

Issue

Built on top of #3781

Unit tests

None included

Code style

Documentation

Transmodel API description added

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 +NeTEx This issue is related to the Netex model/import. !New Feature A functional feature targeting the end user. labels Jan 10, 2022
@t2gran t2gran added this to the 2.1 milestone Jan 24, 2022
@t2gran t2gran modified the milestones: 2.1, 2.2 Feb 25, 2022
@hannesj
hannesj force-pushed the otp2_read_netex_translations branch 2 times, most recently from d461053 to c31b3c8 Compare February 25, 2022 11:15
@hannesj hannesj changed the title Read Netex translations and expose them through the Transmodel API Read translations and expose them ¨ Mar 8, 2022
@hannesj hannesj changed the title Read translations and expose them ¨ Read translations from GTFS and Netex sources for stop names Mar 8, 2022
@hannesj
hannesj marked this pull request as ready for review March 8, 2022 09:41
@hannesj
hannesj requested a review from a team as a code owner March 8, 2022 09:41
@t2gran
t2gran requested review from optionsome and t2gran March 8, 2022 09:48
return getValue(
environment,
stop -> {
return stop.getName().toString(lang == null ? environment.getLocale() : new Locale(lang));

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 think there is quite a bit of duplication here and in the URL. Can you make this a little DRY?

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 uses LegacyGraphQLUtils.getLocale now

@hannesj

hannesj commented Mar 15, 2022

Copy link
Copy Markdown
Contributor Author

At least with the GraphQL API it seems to now default to using the translated language when the requested language doesn't match which seems a bit problematic. Also, there is some issue with null stop URLs when trying to fetch them through the API.

This should be fixed now

hannesj and others added 2 commits March 15, 2022 16:03
@optionsome

Copy link
Copy Markdown
Member

Fetching stop urls when they are not defined in data still causes NPE:

17:28:22.602 WARN (SimpleDataFetcherExceptionHandler.java:27) Exception while fetching data (/stops[1736]/url) : null
java.lang.NullPointerException: null
	at org.opentripplanner.ext.legacygraphqlapi.datafetchers.LegacyGraphQLStopImpl.lambda$url$18(LegacyGraphQLStopImpl.java:123)
	at org.opentripplanner.ext.legacygraphqlapi.datafetchers.LegacyGraphQLStopImpl.getValue(LegacyGraphQLStopImpl.java:464)

optionsome
optionsome previously approved these changes Mar 16, 2022

@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.

Thanks for doing these changes. Everything seems to work now and the code looks good. Just one question, do we need to update the serialization version due to the String -> I18NString type change in the stop model?

@hannesj

hannesj commented Mar 16, 2022

Copy link
Copy Markdown
Contributor Author

Just one question, do we need to update the serialization version due to the String -> I18NString type change in the stop model?

True. Done!

@optionsome

Copy link
Copy Markdown
Member

I think the serialization version was also changed to 19 in dev-2.x, you have to update it again. We could discuss today in the developer meeting if it would be possible to automate the serialization version update similarly as we have done with the changelog (maybe based on some label of a merged pr?).

@hannesj
hannesj merged commit 5fce59c into opentripplanner:dev-2.x Mar 17, 2022
t2gran pushed a commit that referenced this pull request Mar 17, 2022
@hannesj
hannesj deleted the otp2_read_netex_translations branch March 17, 2022 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

+NeTEx This issue is related to the Netex model/import. !New Feature A functional feature targeting the end user.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants