-
Notifications
You must be signed in to change notification settings - Fork 3
Export trips.trip_id
#21
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
Conversation
|
/gemini review |
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.
Code Review
This pull request introduces the functionality to export trips.trip_id, which is a valuable addition. The implementation is well-executed, with new classes MTripId and MTripIds that are consistent with existing patterns for handling IDs. The integration into the data loading, generation, and dumping processes is clean. I appreciate the refactoring done in MSchedule, MFrequency, and MServiceDate to centralize logic into MServiceIds.convert() and MTripIds.convert(), which improves code maintainability. The replacement of the hardcoded EXPORT_TRIP_ID flag with a feature flag is also a good improvement. I have one minor suggestion to improve consistency in naming. Overall, this is a great contribution.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
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.
Pull request overview
This PR adds support for exporting trips.trip_id as integers similar to the existing service ID integer export functionality. The changes enable tracking and converting trip IDs between string and integer representations, controlled by a feature flag.
Key changes:
- Introduces
MTripIddata class andMTripIdsobject for managing trip ID mappings - Adds
convert()methods to bothMTripIdsandMServiceIdsfor conditional formatting - Updates schedule file generation to use the new trip ID conversion system
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| MTripIds.kt | New trip ID management singleton with thread-safe integer mapping and conversion utilities |
| MTripId.kt | New data class for trip ID with integer/string pair and file serialization support |
| MServiceIds.kt | Adds convert() method and renames getString() to getId() for consistency |
| MSchedule.kt | Refactors file output methods into single toFile() with trip ID conversion support |
| MServiceDate.kt | Simplifies service ID conversion using new MServiceIds.convert() method |
| MFrequency.kt | Simplifies service ID conversion using new MServiceIds.convert() method |
| MReader.kt | Adds loadTripIds() method for reading trip ID mappings from files |
| MGenerator.java | Integrates trip ID dumping and updates logging terminology from "Trips" to "Directions" |
| GTime.kt | Removes unused import |
| GFieldTypes.kt | Adds spell-check suppression for time format pattern |
| DumpDbUtils.kt | Adds trip ID table creation/deletion when feature flag enabled |
| DefaultAgencyTools.java | Removes EXPORT_TRIP_ID constant and initializes trip IDs during startup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/gemini review |
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.
Code Review
This pull request successfully implements the export of trips.trip_id, controlled by a new feature flag. The introduction of MTripId and MTripIds is consistent with the existing architecture for handling IDs.
Beyond the main feature, this PR includes valuable refactoring that improves code quality and maintainability:
- Code Consolidation: The file reading logic in
MReader.kthas been nicely refactored into a genericreadFilefunction, reducing code duplication. Similarly, schedule serialization inMSchedule.ktis now handled by a singletoFilemethod. - Bug Fixes & Clarity: A bug in
GSpec.javarelated to service ID cleanup has been fixed, and variable names have been improved for clarity (e.g.,mTripsListtomDirectionsList). - Enhanced Logging: The addition of prefixes to log messages throughout
DefaultAgencyTools.javawill make debugging easier.
I've provided a couple of suggestions to further improve code clarity and fix a minor logging issue. Overall, this is a solid contribution.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
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.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
trips.trip_idcommons-java#11