Docs and CLI Improvements - #471
Merged
Merged
Conversation
The prose in docs/ and the transformer usage.txt did not accurately describe the actual CLI behavior. Rewrite them from the GtfsMergerMain / GtfsTransformerMain source: - merge: drop the non-existent --fuzzyDuplicates / --renameDuplicates flags; document reverse-order precedence, automatic id-prefix renaming, per-file index-pairing of --duplicateDetection, --debug, and the --file requirement for the log/error flags. Reworked the service-change example accordingly. - transformer: list the full flag set (--modifications alias, --reference, built-in transform flags, file-parameter flags) and note command-line order. - bump documented Java runtime to 25 to match the build target.
EDuplicateRenamingStrategy.AGENCY (prefix colliding ids with "<agency_id>-"
instead of the context-based "a-", "b-", ...) has been settable via the Java
API since 2015 and is exercised by GtfsMergerTest's Puget Sound tests, but no
CLI ever exposed it. The Puget Sound consolidated bundle depends on it, so
reproducing that bundle from the command line was impossible.
The new option is index-paired with --file, matching --duplicateDetection:
--file=stops.txt --duplicateDetection=none --duplicateRenaming=agency
This commit also contains end-to-end coverage of the new option. Two feeds
with colliding stop and trip ids are merged and the output feed is read back.
- agency renaming prefixes the lower-priority feed's colliding stop with
"<agency_id>-" ("3-100")
- omitting the option preserves the historical context prefix ("a-100")
- values are index-paired with --file, so stops and trips can use different
renaming strategies in one invocation
This is the merge-cli module's first test, so the pom gains a test-scoped
junit-jupiter-api dependency (version managed by the parent).
📝 WalkthroughWalkthroughAdds a ChangesMerge CLI duplicate renaming feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Transformer CLI documentation updates
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/onebusaway-gtfs-merge-cli.md`:
- Line 23: Add a language identifier to each fenced code block in the markdown
so markdownlint MD040 passes. Update the fences in the docs content around the
onebusaway-gtfs-merge-cli examples to use a shell language tag consistently for
the blocks at the referenced locations, keeping the existing command text
unchanged.
- Around line 34-42: The merge CLI docs currently describe only the default
context-prefix renaming and the per-`--file` pairing logic, but they never
mention the new `--duplicateRenaming` option introduced by
`GtfsMergerMain.buildMerger()`. Update this section to document
`--duplicateRenaming` alongside `--duplicateDetection`, explain that it is
supplied per input index via `--file`-indexed options, and list the supported
values (`context` and `agency`) so users can discover how to switch from the
default context-based renaming to agency-based renaming.
- Around line 56-66: The recognized GTFS files list is missing support for
areas, even though GtfsMergerMain already accepts Area through
AreaMergeStrategy. Update the documented supported-files section to include
areas.txt alongside the other recognized inputs, keeping the list aligned with
the merger’s actual capabilities and the Area-related symbols used in the CLI.
In `@onebusaway-gtfs-merge-cli/pom.xml`:
- Around line 29-33: The test dependency setup in pom.xml only adds
junit-jupiter-api, so GtfsMergerMainTest still won’t execute because the JUnit 5
engine is missing. Update the test dependencies in this module to include
junit-jupiter-engine, or replace the current JUnit 5 API dependency with the
junit-jupiter aggregate, while keeping the existing parent-managed versioning
intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 18448845-dbde-422f-b473-33907259b7e3
📒 Files selected for processing (6)
docs/onebusaway-gtfs-merge-cli.mddocs/onebusaway-gtfs-transformer-cli.mdonebusaway-gtfs-merge-cli/pom.xmlonebusaway-gtfs-merge-cli/src/main/java/org/onebusaway/gtfs_merge/GtfsMergerMain.javaonebusaway-gtfs-merge-cli/src/test/java/org/onebusaway/gtfs_merge/GtfsMergerMainTest.javaonebusaway-gtfs-transformer-cli/src/main/resources/org/onebusaway/gtfs_transformer/usage.txt
|
|
||
| To run the application: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language identifier to fenced code blocks.
markdownlint flags these three blocks (MD040) for missing a language.
📝 Proposed fix
-```
+```shell
java -jar onebusaway-gtfs-merge-cli.jar [options] input_gtfs_path_a input_gtfs_path_b ... output_gtfs_pathApply the same fix at lines 49 and 125.
Also applies to: 49-49, 125-125
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 23-23: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/onebusaway-gtfs-merge-cli.md` at line 23, Add a language identifier to
each fenced code block in the markdown so markdownlint MD040 passes. Update the
fences in the docs content around the onebusaway-gtfs-merge-cli examples to use
a shell language tag consistently for the blocks at the referenced locations,
keeping the existing command text unchanged.
Source: Linters/SAST tools
Comment on lines
+34
to
+42
| ### How feeds are combined | ||
|
|
||
| Input feeds are processed in **reverse command-line order** — entities from the *last* feed listed are | ||
| added to the output first, and entities from earlier feeds are merged in afterward. When an entity | ||
| from an earlier feed collides with one already in the output and is *not* treated as a duplicate (see | ||
| below), its id is automatically renamed by prefixing it (e.g. `a-`, `b-`, …), and all references to it | ||
| are rewritten. This automatic renaming replaces the old `--renameDuplicates` flag, which no longer | ||
| exists. | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Missing documentation for the new --duplicateRenaming option.
This section documents the automatic context-prefix renaming and the --file/--duplicateDetection per-index pairing, but never mentions --duplicateRenaming at all — the very option this PR introduces to let users pick agency-based renaming instead of the default context prefix. GtfsMergerMain.buildMerger() pairs duplicateRenamingOptions by index exactly like duplicateDetectionOptions (same --file-indexed pattern), so readers have no way to discover this flag or its context/agency values from the docs.
📝 Suggested addition
Input feeds are processed in **reverse command-line order** — entities from the *last* feed listed are
added to the output first, and entities from earlier feeds are merged in afterward. When an entity
from an earlier feed collides with one already in the output and is *not* treated as a duplicate (see
below), its id is automatically renamed by prefixing it (e.g. `a-`, `b-`, …), and all references to it
are rewritten. This automatic renaming replaces the old `--renameDuplicates` flag, which no longer
exists.
+
+By default, colliding ids are renamed using the context prefix above. You can instead rename them
+using the owning agency's id with `--duplicateRenaming=agency`, paired by position with `--file` just
+like `--duplicateDetection` (see below). `--duplicateRenaming=context` restores the default explicitly.
+Values are case-insensitive.As per the PR objectives, "It exposes the --duplicateRenaming option in the merge CLI, allowing agency-based duplicate renaming to be selected from the command line," so the docs rewrite should cover it.
Also applies to: 43-70
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/onebusaway-gtfs-merge-cli.md` around lines 34 - 42, The merge CLI docs
currently describe only the default context-prefix renaming and the per-`--file`
pairing logic, but they never mention the new `--duplicateRenaming` option
introduced by `GtfsMergerMain.buildMerger()`. Update this section to document
`--duplicateRenaming` alongside `--duplicateDetection`, explain that it is
supplied per input index via `--file`-indexed options, and list the supported
values (`context` and `agency`) so users can discover how to switch from the
default context-based renaming to agency-based renaming.
Comment on lines
56
to
+66
| - `agency.txt` | ||
| - `stops.txt` | ||
| - `routes.txt` | ||
| - `trips.txt` and `stop_times.txt` | ||
| - `calendar.txt` and `calendar_dates.txt` | ||
| - `calendar.txt` and `calendar_dates.txt` | ||
| - `shapes.txt` | ||
| - `fare_attributes.txt` | ||
| - `fare_rules.txt` | ||
| - `frequencies.txt` | ||
| - `transfers.txt` | ||
|
|
||
| You can specify merge options for each of these files using the `--file=gtfs_file.txt` option. File types listed | ||
| together (eg. `trips.txt` and `stop_times.txt`) are handled by the same merge strategy, so specifying options for | ||
| either will have the same effect. For details on options you might specify, read on. | ||
| - `fare_attributes.txt` | ||
| - `fare_rules.txt` | ||
| - `feed_info.txt` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all CsvFields filename declarations to compare against the documented list
rg -n 'filename\s*=\s*"' --type java -g '!**/test/**'Repository: OneBusAway/onebusaway-gtfs-modules
Length of output: 172
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate likely GTFS merger/doc sources and map their structure.
git ls-files | rg '(^|/)(docs/onebusaway-gtfs-merge-cli\.md|.*GtfsMerger.*|.*GtfsEntitySchemaFactory.*|.*entityClassesByFilename.*|.*MergeStrategy.*)$'
printf '\n## Outline: docs/onebusaway-gtfs-merge-cli.md\n'
wc -l docs/onebusaway-gtfs-merge-cli.md
sed -n '1,220p' docs/onebusaway-gtfs-merge-cli.md
printf '\n## Search: filenames / recognized files / merge strategies\n'
rg -n 'entityClassesByFilename|getEntityClasses|Recognized files|merge strategy|MergeStrategy|filename' src test . -g '!**/target/**' -g '!**/build/**'Repository: OneBusAway/onebusaway-gtfs-modules
Length of output: 45058
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If the search above was too broad, narrow down by likely package names.
fd -a 'GtfsMergerMain.*|GtfsMerger.*|GtfsEntitySchemaFactory.*' .Repository: OneBusAway/onebusaway-gtfs-modules
Length of output: 812
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## GtfsEntitySchemaFactory\n'
sed -n '1,220p' onebusaway-gtfs/src/main/java/org/onebusaway/gtfs/serialization/GtfsEntitySchemaFactory.java
printf '\n## GtfsMergerMain\n'
sed -n '1,260p' onebusaway-gtfs-merge-cli/src/main/java/org/onebusaway/gtfs_merge/GtfsMergerMain.java
printf '\n## GtfsMerger\n'
sed -n '1,320p' onebusaway-gtfs-merge/src/main/java/org/onebusaway/gtfs_merge/GtfsMerger.javaRepository: OneBusAway/onebusaway-gtfs-modules
Length of output: 20324
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## GtfsEntitySchemaFactoryTest\n'
sed -n '1,220p' onebusaway-gtfs/src/test/java/org/onebusaway/gtfs/serialization/GtfsEntitySchemaFactoryTest.java
printf '\n## GtfsMergerMainTest\n'
sed -n '1,260p' onebusaway-gtfs-merge-cli/src/test/java/org/onebusaway/gtfs_merge/GtfsMergerMainTest.java
printf '\n## GtfsMergerTest relevant strategy assertions\n'
sed -n '250,380p' onebusaway-gtfs-merge/src/test/java/org/onebusaway/gtfs_merge/GtfsMergerTest.javaRepository: OneBusAway/onebusaway-gtfs-modules
Length of output: 13058
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## Merge strategy entity-type mappings\n'
rg -n 'getEntityTypes|new .*MergeStrategy|extends Abstract' onebusaway-gtfs-merge/src/main/java/org/onebusaway/gtfs_merge/strategies -g '!**/target/**'
printf '\n## TripMergeStrategy\n'
sed -n '1,220p' onebusaway-gtfs-merge/src/main/java/org/onebusaway/gtfs_merge/strategies/TripMergeStrategy.java
printf '\n## ServiceCalendarMergeStrategy\n'
sed -n '1,220p' onebusaway-gtfs-merge/src/main/java/org/onebusaway/gtfs_merge/strategies/ServiceCalendarMergeStrategy.java
printf '\n## AreaMergeStrategy\n'
sed -n '1,160p' onebusaway-gtfs-merge/src/main/java/org/onebusaway/gtfs_merge/strategies/AreaMergeStrategy.javaRepository: OneBusAway/onebusaway-gtfs-modules
Length of output: 14539
Add areas.txt to the recognized-files list GtfsMergerMain accepts Area via AreaMergeStrategy, so this section is currently missing a supported GTFS file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/onebusaway-gtfs-merge-cli.md` around lines 56 - 66, The recognized GTFS
files list is missing support for areas, even though GtfsMergerMain already
accepts Area through AreaMergeStrategy. Update the documented supported-files
section to include areas.txt alongside the other recognized inputs, keeping the
list aligned with the merger’s actual capabilities and the Area-related symbols
used in the CLI.
This was referenced Jul 5, 2026
leonardehrenfried
approved these changes
Jul 6, 2026
Collaborator
|
Released as 14.1.0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
--duplicateRenamingoption to merge CLISummary by CodeRabbit
New Features
Documentation
Tests