JSON config > + additional_languages & stop_code_cleaners#66
JSON config > + additional_languages & stop_code_cleaners#66
additional_languages & stop_code_cleaners#66Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for additional languages in the agency configuration and implements stop code cleaning logic. The changes include updates to language retrieval and the addition of a cleaning step for stop codes. Feedback focuses on ensuring consistent return values for language lists, applying stop code cleaning to all source fields, and optimizing the performance of language merging to avoid redundant object creation and duplicates.
I am having trouble creating individual review comments. Click here to see my feedback.
src/main/java/org/mtransit/parser/DefaultAgencyTools.java (204-208)
The current implementation is inconsistent regarding the return value when no languages are supported. If agencyConfig is present but the list is empty, getAllLanguages returns null. However, if agencyConfig is absent, it returns this.supportedLanguages (which is an empty list). To maintain consistency with the @Nullable annotation and previous behavior (where null signified that no languages were set), it should return null when the list is empty in both branches.
if (Configs.getAgencyConfig() != null) {
return Configs.getAgencyConfig().getAllLanguages(this.supportedLanguages);
}
return this.supportedLanguages.isEmpty() ? null : this.supportedLanguages;src/main/java/org/mtransit/parser/DefaultAgencyTools.java (1276-1278)
The stop_code_cleaners should be applied to the final stop code value, regardless of whether it originates from the stop_code or stop_id field (when use_stop_id_for_stop_code is enabled). This ensures that any cleaning logic defined in the configuration is consistently applied to the value displayed as the stop code in the UI.
final String stopCode = cleanStopCode(
Configs.getRouteConfig().getUseStopIdForStopCode() ? gStop.getStopId() : gStop.getStopCode()
);
src/main/java/org/mtransit/parser/config/gtfs/data/AgencyConfig.kt (65-68)
This method is called frequently during the parsing process (e.g., for every route and stop name cleaning). Repeatedly mapping additionalLanguages to Locale objects and creating new lists via the + operator is inefficient. Additionally, additionalLanguages might contain duplicates of supportedLanguages, which should be handled to avoid redundant processing downstream. Caching the merged list or at least the additionalLanguages locales would significantly improve performance.
fun getAllLanguages(supportedLanguages: List<Locale>?): List<Locale>? {
if (supportedLanguages.isNullOrEmpty()) return null
if (additionalLanguages.isEmpty()) return supportedLanguages
return (supportedLanguages + additionalLanguages.map { Locale.forLanguageTag(it) }).distinct()
}There was a problem hiding this comment.
Pull request overview
This PR extends the GTFS JSON configuration surface to support (1) expanding an agency’s supported languages via additional_languages, and (2) applying configurable stop-code cleanup via stop_code_cleaners, then wires these features into DefaultAgencyTools.
Changes:
- Add
AgencyConfig.additional_languagesand merge it into the tool’s supported languages list. - Add
RouteConfig.stop_code_cleaners+cleanStopCode()and apply it inDefaultAgencyTools.getStopCode(). - Minor clarification comment in
GReaderabout reading agency first to set up languages.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/main/java/org/mtransit/parser/gtfs/GReader.java | Clarifies agency read ordering for language setup. |
| src/main/java/org/mtransit/parser/DefaultAgencyTools.java | Merges additional languages (cached) and cleans stop codes via route config. |
| src/main/java/org/mtransit/parser/config/gtfs/data/RouteConfig.kt | Adds stop_code_cleaners and cleanStopCode() helper. |
| src/main/java/org/mtransit/parser/config/gtfs/data/AgencyConfig.kt | Adds additional_languages and helper to merge with GTFS-provided languages. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
/gemimi review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a caching mechanism for supported languages in DefaultAgencyTools and introduces stop code cleaning capabilities. The review feedback identifies concurrency issues in the caching logic, recommending the use of volatile and a more robust access pattern to prevent race conditions. Other suggestions include adhering to the Google Java Style Guide by using braces for all if statements and improving code formatting for better readability.
- commons: Generate `fr/gtfs_rts_strings.xml` & ignore location inside agency name. mtransitapps/commons#730 - commons-java: Strings cleaner > FR > remove `ligne` from route long name & `station` from trip head-sign mtransitapps/commons-java#34 - parser: JSON config > + `additional_languages` & `stop_code_cleaners` mtransitapps/parser#66
- commons: Generate `fr/gtfs_rts_strings.xml` & ignore location inside agency name. mtransitapps/commons#730 - commons: Build(deps): Bump androidx.webkit:webkit from 1.15.0 to 1.16.0 in the androidx group across 1 directory mtransitapps/commons#731 - commons-java: Strings cleaner > FR > remove `ligne` from route long name & `station` from trip head-sign mtransitapps/commons-java#34 - parser: JSON config > + `additional_languages` & `stop_code_cleaners` mtransitapps/parser#66
Uh oh!
There was an error while loading. Please reload this page.