-
Notifications
You must be signed in to change notification settings - Fork 2
Export trips.trip_id
#42
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 support for exporting and using trip_id from GTFS data, which involves modifications across data models, database helpers, and providers. The changes are generally well-structured and align with the feature's goal. However, I've identified a few issues that need attention: a minor bug in a toString() implementation, a more significant problem with the progress reporting logic during database setup, and an instance of code duplication that could impact future maintenance. My review provides specific feedback to address these points.
src/main/java/org/mtransit/android/commons/provider/GTFSProviderDbHelper.java
Outdated
Show resolved
Hide resolved
src/main/java/org/mtransit/android/commons/provider/GTFSStatusProvider.java
Outdated
Show resolved
Hide resolved
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/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 support for exporting trip_id and arrival times, which involves changes to data structures, database helpers, and data parsing logic. The addition of GTFSTripIdsUtils.kt for resolving trip IDs is a good, clean implementation. I have identified a few areas for improvement related to code readability, maintainability, and robustness. My comments include suggestions for refactoring complex string concatenation, using utility methods for consistency, and improving the initialization of static constants.
src/main/java/org/mtransit/android/commons/provider/GTFSStatusProvider.java
Outdated
Show resolved
Hide resolved
src/main/java/org/mtransit/android/commons/provider/GTFSStatusProvider.java
Outdated
Show resolved
Hide resolved
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 pull request adds support for exporting trips.trip_id from GTFS data, building on the existing string export functionality from commons-java#11. The implementation includes database schema changes, file parsing updates, and data model enhancements.
Key changes:
- Added new
GTFSTripIdsUtilsutility class to handle trip ID lookups and conversions - Extended
Schedule.Timestampdata model withtripIdandarrivalDiffMsfields - Refactored SQL string handling methods for consistency (
unescapeStringOrNull→unquotesUnescapeStringOrNull)
Reviewed changes
Copilot reviewed 9 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/org/mtransit/android/commons/provider/gtfs/GTFSTripIdsUtils.kt | New utility class for loading and updating trip IDs from GTFS database |
| src/main/java/org/mtransit/android/commons/data/Schedule.java | Added tripId and arrivalDiffMs fields to Timestamp class with supporting methods |
| src/main/java/org/mtransit/android/commons/provider/GTFSStatusProvider.java | Updated schedule parsing logic to handle trip IDs and arrival times when feature flag enabled |
| src/main/java/org/mtransit/android/commons/provider/GTFSScheduleTimestampsProvider.java | Added trip ID update call for schedule timestamps |
| src/main/java/org/mtransit/android/commons/provider/GTFSProviderDbHelper.java | Added trip_ids table schema and initialization logic |
| src/main/java/org/mtransit/android/commons/provider/gtfs/GTFSStringsUtils.kt | Updated package declaration and imports |
| src/main/java/org/mtransit/android/commons/SqlUtils.java | Renamed method from unescapeStringOrNull to unquotesUnescapeStringOrNull |
| src/test/java/org/mtransit/android/commons/SqlUtilsTest.kt | Updated test to reflect renamed method |
| src/main/java/org/mtransit/android/commons/provider/gtfs/GTFSProviderDBHelperUtils.kt | Removed unused import |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/main/java/org/mtransit/android/commons/provider/GTFSStatusProvider.java
Outdated
Show resolved
Hide resolved
src/main/java/org/mtransit/android/commons/provider/gtfs/GTFSTripIdsUtils.kt
Show resolved
Hide resolved
|
/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 support for trip_id and arrival times in schedules, which involves changes across the data model, database, and data parsing logic. The changes are well-structured and cover all necessary aspects from data storage to representation. The PR also includes several bug fixes and refactorings. My review focuses on improving maintainability and fixing a minor UI bug in the progress notification.
src/main/java/org/mtransit/android/commons/provider/GTFSProviderDbHelper.java
Outdated
Show resolved
Hide resolved
src/main/java/org/mtransit/android/commons/provider/GTFSStatusProvider.java
Show resolved
Hide resolved
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 12 out of 17 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/main/java/org/mtransit/android/commons/provider/GTFSStatusProvider.java:1
- The method
MTLog.formatDurationis called but the new function added in this PR isformatSimpleDurationin TimeUtils. Verify thatMTLog.formatDurationexists or if this should callTimeUtils.formatSimpleDuration(durationInMs)instead.
package org.mtransit.android.commons.provider;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
trips.trip_idcommons-java#11